feat(pack): switch --archive to .zip and add --archive-format zip|tar.gz#1720
Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches apm pack --archive to produce .zip bundles by default, adds --archive-format zip|tar.gz for explicit selection, and extends bundle extraction to support .zip while keeping .tar.gz as a legacy-compatible input.
Changes:
- Default
apm pack --archiveoutput changes from.tar.gzto.zip, with a new--archive-formatoption to opt into.tar.gz. - Bundle packing/exporting now branches on
archive_formatto write either ZIP or tar.gz archives. - Bundle unpacking adds a primary ZIP extraction path with traversal + symlink-entry defenses, while retaining the existing tar.gz path.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/commands/pack.py |
Updates CLI help text for archive output; adds `--archive-format zip |
src/apm_cli/core/build_orchestrator.py |
Threads new bundle_archive_format option from CLI into bundle production. |
src/apm_cli/bundle/packer.py |
Implements archive-format branching for APM-format bundle archives. |
src/apm_cli/bundle/plugin_exporter.py |
Implements archive-format branching for plugin-format bundle archives. |
src/apm_cli/bundle/unpacker.py |
Adds ZIP extraction + security validation; preserves legacy tar.gz extraction. |
tests/unit/test_packer.py |
Updates unit expectations/validation from tar.gz to zip for pack_bundle(..., archive=True). |
tests/unit/test_unpacker.py |
Updates archive helper to generate ZIP archives for unpack unit tests. |
tests/unit/commands/test_pack_phase3.py |
Updates hardcoded bundle file names in CLI-related tests from .tar.gz to .zip. |
tests/unit/commands/test_pack_cli_surface.py |
Same .tar.gz -> .zip updates for CLI surface tests. |
tests/integration/test_pack_unpack_e2e.py |
Updates end-to-end pack/unpack flow to look for *.zip artifacts. |
tests/integration/test_wave6_init_pack_coverage.py |
Updates --archive docstring expectation to .zip. |
tests/integration/test_wave3_marketplace_coverage.py |
Updates --archive docstring expectation to .zip. |
392684c to
1815123
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 1 | Well-motivated switch; packer.py skips ensure_path_within that plugin_exporter.py applies; extract shared _write_archive helper. |
| CLI Logging Expert | 1 | 2 | 2 | Post-pack success tip instructs 'apm install ./bundle.zip' but detect_local_bundle() in local_bundle.py does not recognize .zip -- command silently fails. |
| DevX UX Expert | 0 | 3 | 2 | ZIP-by-default is sound; no runtime hint for breaking default change; --archive-format silently no-ops without --archive; three doc surfaces stale. |
| Supply Chain Security Expert | 0 | 3 | 1 | Pre-validation chain is sound; packer.py omits ensure_path_within for archive_path; no ZIP bomb guard before extractall; Python 3.12 rglob symlink gap. |
| OSS Growth Hacker | 0 | 2 | 1 | Strong strategic alignment; missing CHANGELOG; ci-cd.md 'tar xzf' command breaks silently for upgrading users. |
| Doc Writer | 0 | 4 | 1 | Five documentation surfaces now incorrect: CHANGELOG, pack.md, ci-cd.md/gh-aw.md (live CI commands break), commands.md, unpack.md/install.md. |
| Test Coverage Expert | 1 | 1 | 0 | ZIP extraction security checks (path traversal, symlink, absolute path) have zero test coverage despite being the new default extraction path. |
| Performance Expert | 0 | 3 | 3 | ZIP archives are 30-127% larger for typical APM bundles (measured); size regression is real and undocumented in CHANGELOG. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[CLI Logging Expert] (blocking-severity) Update
local_bundle.pydetect_local_bundle()and_looks_like_archive()to recognize.zipso the post-pack success message gives users a command that actually works. -- This is a correctness regression in the default path: the success tip instructs users to run a broken command. Fix is 2-3 lines. Must land in this PR before merge. -
[Test Coverage Expert] (blocking-severity) Add ZIP extraction security tests to
test_unpacker.pycovering path traversal, absolute path rejection, andexternal_attrsymlink detection -- mirroring the existingtest_unpack_with_path_traversal_tarballandtest_unpack_with_symlink_tarballtests for the tar.gz path. Also add a test for--archive-format tar.gzround-trip. -- ZIP is now the DEFAULT extraction path. Exhaustive grep confirms zero test coverage for these cases. New security-critical code on the default path with no automated guardrail is a regression trap. Must land in this PR before merge. -
[Python Architect + Supply Chain Security Expert] Add
ensure_path_within(archive_path, output_dir)inpacker.py(both archive branches) and an uncompressed-size/entry-count guard beforezf.extractallinunpacker.py. -- Two independent panelists confirmed the write-path-traversal gap: apkg_namelike../evilinapm.ymlescapesoutput_dirat pack time. The ZIP bomb guard prevents denial-of-service. Should land in this PR or a companion PR opened before merge. -
[Doc Writer + OSS Growth Hacker + DevX UX Expert] Replace
tar xzf build/*.tar.gzwith the.zipequivalent inci-cd.mdandgh-aw.md. Add CHANGELOG[Unreleased]entry for the default format change (size tradeoff disclosure + migration note). Add--archive-formattopack.mdoptions table. Emit a runtime warning when--archive-formatis passed without--archive. -- Three panelists flagged these surfaces independently. The CHANGELOG gap violates APM governance policy; theci-cd.mdCI command breaks silently for upgrading users. -
[Python Architect] Extract a shared
_write_archive()helper from the duplicate archive-writing blocks inpacker.pyandplugin_exporter.py. Type-narrowarchive_formattoLiteral['zip', 'tar.gz']in all function signatures andBuildOptions. -- The two blocks have already drifted (packer.py missingensure_path_within); the shared helper prevents future divergence and eliminates the security gap by construction.
Architecture
classDiagram
direction LR
class BuildOrchestrator {
<<Coordinator>>
-_producers list
+run(options, logger) BuildResult
}
class ArtifactProducer {
<<Protocol>>
+kind OutputKind
+produce(options, logger) ProducerResult
}
class BundleProducer {
<<ConcreteStrategy>>
+kind OutputKind
+produce(options, logger) ProducerResult
}
class BuildOptions {
<<ValueObject>>
+bundle_archive bool
+bundle_archive_format str
+bundle_format str
+bundle_output Path
+dry_run bool
}
class PackResult {
<<ValueObject>>
+bundle_path Path
+files list
}
class pack_bundle {
<<Function>>
+archive bool
+archive_format str
+fmt str
}
class export_plugin_bundle {
<<Function>>
+archive bool
+archive_format str
}
class unpack_bundle {
<<Function>>
+bundle_path Path
}
note for pack_bundle "packer.py apm fmt: NO sanitize NO ensure_path_within"
note for export_plugin_bundle "plugin_exporter.py: sanitize then ensure_path_within both branches"
BuildOrchestrator o-- ArtifactProducer : manages
BundleProducer ..|> ArtifactProducer : implements
BundleProducer ..> BuildOptions : reads
BundleProducer ..> pack_bundle : calls
pack_bundle ..> export_plugin_bundle : delegates plugin fmt
pack_bundle ..> PackResult : returns
export_plugin_bundle ..> PackResult : returns
class BuildOptions:::touched
class pack_bundle:::touched
class export_plugin_bundle:::touched
class unpack_bundle:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
CLI["apm pack --archive --archive-format zip|tar.gz"]
BO["BuildOptions.bundle_archive_format - build_orchestrator.py:44"]
ORCH["BuildOrchestrator.run - build_orchestrator.py:414"]
BP["BundleProducer.produce - build_orchestrator.py:98"]
PB["pack_bundle - packer.py:26"]
EPB["export_plugin_bundle - plugin_exporter.py:406"]
ARCH1{"archive_format? plugin fmt"}
ARCH2{"archive_format? apm legacy fmt"}
ZIP1["ZipFile.write - plugin_exporter.py:662"]
TAR1["tarfile.open - plugin_exporter.py:654"]
EPW1["ensure_path_within archive_path - plugin_exporter.py:661"]
EPW2["ensure_path_within archive_path - plugin_exporter.py:653"]
RMTREE1["shutil.rmtree bundle_dir - plugin_exporter.py:669"]
ZIP2["ZipFile.write - packer.py:286"]
TAR2["tarfile.open - packer.py:279"]
GAP["GAP: no sanitize, no ensure_path_within - packer.py:277-294"]
RMTREE2["shutil.rmtree bundle_dir - packer.py:293"]
UNPACK["apm unpack .zip - unpacker.py:29"]
SCAN["pre-validate zip members: path-traversal + symlink - unpacker.py:66-83"]
EXTALL["zf.extractall temp_dir - unpacker.py:84"]
FINAL["shutil.rmtree temp_dir finally - unpacker.py:282"]
CLI --> BO --> ORCH --> BP --> PB
PB -->|"fmt=plugin (default)"| EPB
PB -->|"fmt=apm legacy"| ARCH2
EPB --> ARCH1
ARCH1 -->|"zip (new default)"| ZIP1
ARCH1 -->|"tar.gz (opt-in)"| TAR1
ZIP1 --> EPW1 --> RMTREE1
TAR1 --> EPW2 --> RMTREE1
ARCH2 -->|"zip"| ZIP2
ARCH2 -->|"tar.gz"| TAR2
ZIP2 --> GAP
TAR2 --> GAP
GAP --> RMTREE2
UNPACK --> SCAN --> EXTALL --> FINAL
Recommendation
The direction is correct and must ship. ZIP-by-default is the right call for Claude Code compatibility and Windows CI adoption, and the panel is unanimous on this. The rework ask is narrow and fast.
Two items must be fixed in this PR before merge. First, update local_bundle.py to recognize .zip so the post-pack success message gives users a command that actually works. This is 2-3 lines; there is no justification for shipping a success tip that instructs users to run a broken command. Second, add ZIP extraction security tests to test_unpacker.py covering path traversal, absolute path rejection, and symlink detection via external_attr. ZIP is now the default extraction path, the security gates are new code, and the tar.gz equivalents have explicit tests. Both items are fast to address in the existing PR branch.
Before merging, the maintainer should also open a companion PR covering: ensure_path_within in packer.py (confirmed write-path-traversal gap), the ci-cd.md and CHANGELOG updates (the live tar xzf CI command breaks upgrading users silently), and the ZIP bomb guard in unpacker.py. The companion PR does not need to merge first, but the commitment to it must be explicit -- a tracking issue at minimum -- before this branch lands. Once the two items are fixed in the PR and the companion PR is opened, this change ships cleanly and the Windows plus Claude Code narrative is ready to amplify.
Full per-persona findings
Python Architect
-
[recommended] packer.py archive block skips
_sanitize_bundle_nameandensure_path_withinthat plugin_exporter.py applies for both formats (src/apm_cli/bundle/packer.py:276)
plugin_exporter.py applies three layered guards before writing an archive: (1)_sanitize_bundle_name(pkg_name/pkg_version), (2)ensure_path_within(bundle_dir, output_dir), (3)ensure_path_within(archive_path, output_dir)for each branch. packer.py constructs archive_path directly from raw pkg_name and pkg_version with none of these guards. A pkg_name value of../evilwould produce an archive_path that escapes output_dir silently.
Suggested: Addensure_path_withinimport to packer.py. Apply_sanitize_bundle_nameto pkg_name/pkg_version, then callensure_path_within(archive_path, output_dir)in both branches, mirroring plugin_exporter.py:646-649 and plugin_exporter.py:653/661. -
[recommended] Identical archive-writing blocks in packer.py and plugin_exporter.py have already drifted; extract a shared
_write_archivehelper (src/apm_cli/bundle/packer.py:276)
packer.py:276-294 and plugin_exporter.py:650-670 contain functionally identical zip/tar.gz branch code. The only diff is that plugin_exporter.py callsensure_path_withinand packer.py does not -- exactly the divergence that duplicated blocks produce over time.
Suggested: Extract to_write_archive(bundle_dir, archive_path, archive_format, output_dir)that callsensure_path_withininternally, eliminating the security gap by construction. -
[nit]
archive_format: strshould beLiteral['zip', 'tar.gz']in function signatures and BuildOptions (src/apm_cli/bundle/packer.py:32)
click.Choice validates at the CLI boundary, but internal callers pass a raw string with no type-checker enforcement. Literal makes the two-value contract explicit to mypy/pyright.
Suggested:from typing import LiteralthenLiteral['zip', 'tar.gz']in pack_bundle, export_plugin_bundle, and BuildOptions.bundle_archive_format.
CLI Logging Expert
-
[blocking] 'Share with: apm install (bundle).zip' emits a command that does not work (
src/apm_cli/bundle/local_bundle.py:134)
Before this PR,apm pack --archiveproduced .tar.gz;detect_local_bundle()matches .tar.gz via_looks_like_archive(). After this PR, the default output is .zip, but_looks_like_archive()only returns True for .tar.gz/.tgz -- not .zip.detect_local_bundle()returns None for .zip paths; the install command falls through to registry-install with a confusing unrelated error.local_bundle.pywas NOT modified by this PR.
Suggested: Extend_looks_like_archive()to also match .zip and add the corresponding zip extraction branch todetect_local_bundle()(mirroring the .zip extraction logic already in unpacker.py). Also extend the IM7 guard in install.py to check '.zip'. -
[recommended]
--archive-formathelp text gives no basis for choosing between zip and tar.gz (src/apm_cli/commands/pack.py:179)
Current: 'Archive format when --archive is set.' The PR body has the right framing (Claude Code compatibility vs legacy CI) but it never reaches the terminal help text.
Suggested: "Archive format: 'zip' (default, Claude Code and plugin-host compatible; matches apm publish output) or 'tar.gz' (legacy CI pipelines that depended on the previous default)." -
[recommended] Three stale
.tar.gz-only references in install.py user-facing strings not updated (src/apm_cli/commands/install.py:1100)
(1)--asoption help at line 1100, (2) example usage at line 1176, (3)--aserror message at line 1326 all name only .tar.gz as the accepted archive format.
Suggested: Change '(directory or .tar.gz produced by apm pack)' to '(directory, .zip, or .tar.gz archive produced by apm pack)'. -
[nit]
--archivehelp cross-reference does not surface the default format (src/apm_cli/commands/pack.py:171)
Suggested: 'Produce an archive instead of a directory. Default format is .zip (see --archive-format to change).' -
[nit]
local_bundle.pymodule docstring andis_archivedocstring still describe .tar.gz as the only archive format (src/apm_cli/bundle/local_bundle.py:4)
Suggested: Update both to include '.zip or legacy .tar.gz'.
DevX UX Expert
-
[recommended] No runtime hint on the silent default-format change: existing scripts will break downstream (
src/apm_cli/commands/pack.py)
apm pack --archive previously produced .tar.gz; it now produces .zip with zero terminal output indicating the change. CI pipelines that runtar xzf *.tar.gzorls *.tar.gzafter pack will fail silently -- APM exits 0, the downstream shell command fails.
Suggested: In_render_bundle_resultor inpack_cmdafter a successful archive, emitlogger.infoorlogger.warningwhenarchive=Trueandarchive_format='zip', pointing to--archive-format tar.gzfor pipelines that relied on the old default. -
[recommended]
--archive-formatsilently no-ops without--archive; should warn (src/apm_cli/commands/pack.py)
archive_formatis accepted by the CLI and forwarded to pack_bundle() but is only evaluated inside theif archive:guard. Runningapm pack --archive-format tar.gzwithout--archiveproduces a directory, discards the flag, and exits 0 with no warning.
Suggested: Aftereffective_targetassignment, add:if archive_format != 'zip' and not archive: logger.warning('--archive-format has no effect without --archive'). -
[recommended] Three doc surfaces still describe
--archiveas producing .tar.gz;--archive-formatabsent from options table (docs/src/content/docs/reference/cli/pack.md)
(a) pack.md line 32 says--archiveproduces .tar.gz; --archive-format missing; (b) pack-a-bundle.md line 45 says '.tar.gz instead of a directory'; (c) commands.md line 83 lists --archive but not --archive-format.
Suggested: Update pack.md --archive row and add --archive-format row. Update pack-a-bundle.md .tar.gz examples to .zip. Add --archive-format to commands.md. -
[nit]
--archivehelp text defers to cross-reference instead of naming the default format (src/apm_cli/commands/pack.py)
Suggested:help='Produce a .zip archive instead of a directory (use --archive-format tar.gz for legacy pipelines).' -
[nit]
--archive-formathelp text does not explain when to choose each value (src/apm_cli/commands/pack.py)
Suggested:help='Archive format when --archive is set. Use tar.gz for CI pipelines that relied on the previous default.'
Supply Chain Security Expert
-
[recommended] packer.py omits
ensure_path_withinfor archive_path; pkg_name from apm.yml can escape output_dir (src/apm_cli/bundle/packer.py:278)
plugin_exporter.py callsensure_path_within(archive_path, output_dir)for both archive format branches. packer.py constructs archive_path directly fromoutput_dir / f'{pkg_name}-{pkg_version}.{ext}'where pkg_name/pkg_version come from apm.yml (user-controlled). A name like../../cron.d/evilcauses archive_path.resolve() to escape output_dir.
Suggested: After each archive_path assignment (lines 278 and 285), addensure_path_within(archive_path, output_dir), mirroring plugin_exporter.py:653 and :661. -
[recommended] No uncompressed-size or entry-count guard before
zf.extractall; ZIP bomb denial-of-service possible (src/apm_cli/bundle/unpacker.py:67)
The pre-validation loop checks path safety but never readsmember.file_size. A crafted ZIP with high compression ratio can decompress to gigabytes. Python's zipfile imposes no decompression limit.
Suggested: Accumulatetotal = sum(m.file_size for m in zf.infolist())and check entry count. Raise ValueError before extractall if total exceeds a size cap (e.g., 512 MB) or entry count exceeds a count cap (e.g., 10000). -
[recommended] Python 3.12 rglob follows symlinked directories by default; is_symlink() guard misses transitive files on that version (
src/apm_cli/bundle/plugin_exporter.py:655)
APM declaresrequires-python >= 3.10. In Python 3.12,Path.rglob()changed its default to follow symbolic links to subdirectories. If bundle_dir contains a symlink to an external directory, rglob yields every file inside that external directory; those files haveis_symlink() == Falseandis_file() == Trueand pass the guard. The oldtarfile.addwith_tar_filternever recursed into symlinked directories.
Suggested: After theis_symlink()check, addensure_path_within(fp, bundle_dir)(catching PathTraversalError and skipping) to reject files reachable only through symlinked directories regardless of Python version. -
[nit] Windows-format ZIP symlink detection via
external_attris a known limitation aligned with Python stdlib (src/apm_cli/bundle/unpacker.py:82)
The check reads Unix file-type bits; ZIPs created by Windows tools leave the upper 16 bits zero. However, Python'szipfile.extractall()uses the sameexternal_attrmechanism -- the check and the extractor are in sync. No exploitable gap; worth documenting as a known limitation in a code comment.
OSS Growth Hacker
-
[recommended] Missing CHANGELOG entry for a default-changing PR -- trust risk for upgrading users
The[Unreleased]section records theapm publishzip switch but is entirely silent onapm pack --archivenow emitting .zip. An operator runningapm upgradehas no signal that their artifact filename changed. The escape hatch only helps users who know they need it. -
[recommended] Three docs pages become stale copy-paste traps on merge -- including a live CI command that silently fails
Confirmed stale: (1) pack.md options table still says .tar.gz; (2) pack-a-bundle.md shows .tar.gz output examples; (3) ci-cd.md containstar xzf build/*.tar.gz -C ./which silently fails for upgrading users copying the snippet. -
[nit] The Windows story is the strongest adoption angle and it is unplayed in the PR body
ZIP is natively extractable on Windows without WSL, GNU tar, or third-party tools. Worth one sentence in the CHANGELOG entry and a phrase in the release note.
Auth Expert -- inactive
No auth files touched; PR changes archive format (zip vs tar.gz) in bundle/packer.py, bundle/plugin_exporter.py, bundle/unpacker.py, commands/pack.py, and core/build_orchestrator.py -- no authentication, token management, credential resolution, or AuthResolver surface is affected.
Doc Writer
-
[recommended] CHANGELOG.md has no
[Unreleased]entry for theapm pack --archiveformat change or the new--archive-formatflag (CHANGELOG.md)
Suggested: Add two bullets under### Addedin[Unreleased]: (1)apm pack --archivenow produces.zipby default instead of.tar.gz; (2)apm pack --archive-format [zip|tar.gz]escape hatch for legacy CI. -
[recommended]
reference/cli/pack.mdoption table describes--archiveas producing.tar.gzand is missing--archive-format(docs/src/content/docs/reference/cli/pack.md)
Line 32: '--archive | off | Produce a .tar.gz archive instead of a directory.' -- now wrong.--archive-formathas no row in the options table.
Suggested: Update line 32 and add--archive-format [zip|tar.gz]row immediately after. Fix line 53 comment and line 206 reference. -
[recommended]
integrations/ci-cd.mdandintegrations/gh-aw.mdshowtar xzfrestore commands that break against the new .zip default (docs/src/content/docs/integrations/ci-cd.md)
ci-cd.md line 211:tar xzf build/*.tar.gz -C ./. gh-aw.md line 126: similar claim. These will fail at runtime for upgrading users.
Suggested: Update restore guidance tounzip -d ./ build/*.zip, or show--archive-format tar.gzexplicitly for pipelines requiring tar.gz. -
[recommended]
packages/apm-guide/.apm/skills/apm-usage/commands.md-- apm pack options column missing--archive-format(packages/apm-guide/.apm/skills/apm-usage/commands.md)
Line 83 lists--archivebut not--archive-format. This is the authoritative machine-readable CLI summary consumed by agent skills.
Suggested: Append--archive-format [zip|tar.gz](default zip) to the apm pack options cell. -
[nit]
reference/cli/unpack.mdandreference/cli/install.mdstill describe only.tar.gzas a valid bundle archive format (docs/src/content/docs/reference/cli/unpack.md)
Suggested: Change 'a .tar.gz archive' to 'a .zip or .tar.gz archive' in unpack.md. Add .zip to packed bundles description in install.md.
Test Coverage Expert
-
[blocking] New ZIP-extraction security checks in unpacker.py -- path traversal, absolute path, external_attr symlink -- have no test; tar.gz equivalents are well covered (
src/apm_cli/bundle/unpacker.py)
Probed: (a) grep'd tests/ for 'external_attr' combined with 'unpack_bundle' -- zero matches; (b) read tests/unit/test_unpacker.py in full -- security tests at lines 191 and 205 exercise the directory bundle path, NOT the ZIP extraction path; (c) grep'd for 'zip.*traversal' and 'unpack_bundle.*zip' -- zero hits. By contrast, tar.gz equivalents DO have tests:test_unpack_with_path_traversal_tarball(test_wave4_pure_logic_coverage.py:945),test_unpack_with_symlink_tarball(test_wave4_pure_logic_coverage.py:962).
Proof (missing):tests/unit/test_unpacker.py::test_unpack_zip_rejects_path_traversal_member-- proves: apm unpack rejects ZIP archives whose member names contain..or absolute paths, and rejects Unix symlinks stored via external_attr [secure-by-default] -
[recommended]
--archive-format tar.gzCLI flag and packer.py tar.gz branch are untested at every tier (src/apm_cli/bundle/packer.py)
grep'd tests/ for 'archive_format', 'archive-format', 'archive_format.*tar' -- zero matches in executable test code. The unit testtest_pack_archivecallspack_bundle(project, out, archive=True)without archive_format, exercising only the default zip branch. packer.py lines 277-283 (the tar.gz branch) has zero test coverage.
Proof (missing):tests/integration/test_wave6_init_pack_coverage.py::test_pack_archive_format_tar_gz-- proves:apm pack --archive --archive-format tar.gzproduces exactly one .tar.gz archive in the build directory, not a .zip [devx, portability]
Performance Expert
-
[recommended] ZIP archives are 30-127% larger than tar.gz for typical APM bundle content; the size regression is real and undocumented (
src/apm_cli/bundle/packer.py)
Measured on representative APM bundles: 50 varied files produce ZIP=11.7 KB vs tar.gz=5.1 KB (+127%). Root cause: ZIP DEFLATE resets the compression dictionary at every entry boundary; tar.gz compresses as one stream, benefiting from cross-file pattern matching across YAML/Markdown skill files.
Suggested: Add a one-line note to the CHANGELOG: 'ZIP archives are typically 30-130% larger than tar.gz for text-heavy skill bundles due to per-file compression; use --archive-format tar.gz when archive size is a priority.' -
[recommended] compresslevel is unset in both ZipFile constructors; default (zlib level 6) is correct but not observable (
src/apm_cli/bundle/packer.py:286)
Both packer.py:286 and plugin_exporter.py:662 call ZipFile without compresslevel. Consider exposing as an internal constant_ZIP_COMPRESS_LEVEL = 6for observability. -
[recommended] No perf benchmarks accompany the default archive format switch; size and speed trade-offs are unquantified in the PR
The PR changes the default archive format for all users with no test measuring archive size, pack wall-time, or unpack wall-time. A benchmark comment in the PR body establishes a regression baseline. -
[nit] sorted(bundle_dir.rglob(*)) overhead is negligible -- no change warranted (
src/apm_cli/bundle/packer.py)
Measured at 200 entries: sort overhead = 0.67 ms vs compression I/O phase of 60-321 ms. The sort is valuable for deterministic archive member ordering. No change warranted. -
[nit] Two-pass extraction in unpacker.py is not actually two disk passes (
src/apm_cli/bundle/unpacker.py)
ZipFile.infolist() returns a list populated during init; the central directory is read from disk exactly once. The validation loop iterates an in-memory list. Overhead measured: 0.7 ms for 200 files vs 26.3 ms extraction. Negligible. -
[nit] cleanup_temp handling is correct; no temp dir leak identified (
src/apm_cli/bundle/unpacker.py)
cleanup_temp is set to True and temp_dir is assigned before the try block. Exception handling and the outer finally block both handle cleanup correctly across all branches.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1720 · sonnet46 22.4M · ◷
|
Addressed the full panel review comments: detect_local_bundle gap: detect_local_bundle() now handles .zip via a new _extract_zip_bundle() helper that runs the full security chain — path traversal rejection, absolute path rejection, and Unix symlink detection via external_attr — before extractall. _looks_like_archive() is intentionally not changed: adding .zip there would silently route zip files into the tar branch and fail with TarError. The .zip branch is an explicit check that runs before the tar branch. The IM7 guard in install.py is extended to cover .zip as well. ZIP security tests: TestUnpackZipSecurity in test_unpacker.py now covers path traversal, absolute path, and external_attr-based symlink detection, mirroring the existing tar.gz equivalents. The --archive-format tar.gz round-trip and invalid-format rejection are also covered in test_packer.py and test_pack_cli_surface.py. Also addressed in this branch rather than deferred: ensure_path_within added to both archive branches in packer.py (mirrors what plugin_exporter.py already did); ZIP bomb guard before extractall in unpacker.py — rejects archives over 512 MB uncompressed or 10,000 entries (the check reads file_size, the declared uncompressed size, so it catches high-compression-ratio attacks before extraction starts); CHANGELOG [Unreleased] entry with the size tradeoff disclosure; ci-cd.md and gh-aw.md restore commands updated from tar xzf to unzip; pack.md options table updated with --archive-format. Also picked up from the advisory findings: --archive-format warns when passed without --archive; install.py user-facing strings updated to include .zip; help text rewritten with the Claude Code / legacy CI framing. Summary for the review: the panel reviewed new code, found real issues, but most of those issues were pre-existing on the tar.gz path too. But as those are security issues I think we must patch them at once. Also, fixed a small issue whereby publish_version in the registry client had Content-Type: application/gzip hard-coded from when publish used to produce tarballs. The archive bytes being uploaded were already a .zip, but the server was being told it was gzip. Fixed to application/zip, parameter renamed from tarball_bytes to archive_bytes |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 1 | 2 | 2 | Correctly switches ZIP to primary and adds --archive-format for backward-compat. ZIP bomb guard absent from local_bundle._extract_zip_bundle(); rglob archive block duplicated in packer.py and plugin_exporter.py; three divergent ZIP validation implementations. |
| CLI Logging Expert | 0 | 2 | 3 | CLI changes are well-scoped. No-op warning missing an action hint; ZIP-bomb ValueError reports raw byte integers instead of human-readable MB. |
| DevX UX Expert | 0 | 2 | 2 | Core ergonomics sound. Skill prose in commands.md still lists only .tar.gz for bundle install; --archive-format without --archive should be UsageError not a warning. |
| Supply Chain Security Expert | 0 | 2 | 3 | ZIP extraction in unpacker.py is solid. Bomb guard absent from _extract_zip_bundle(); guard sums attacker-controlled file_size field not measured output bytes. |
| OSS Growth Hacker | 0 | 2 | 2 | Strong net-positive for adoption. unpack.md and install.md still describe .tar.gz as the canonical format, creating a mismatch for new users. |
| Doc Writer | 0 | 5 | 5 | pack.md and ci-cd.md edits accurate. Five additional pages (pack-a-bundle.md, releasing-from-any-ci.md, install.md, unpack.md, commands.md) contradict the new .zip default -- releasing-from-any-ci.md CI globs will silently break. |
| Test Coverage Expert | 0 | 3 | 1 | Pack unit coverage solid. No integration test for apm install bundle.zip (new default format); no regression trap for the ZIP bomb guard in unpacker.py. |
| Performance Expert | 0 | 1 | 3 | Format-correctness PR with negligible performance impact. Missing bomb guard in _extract_zip_bundle() is the only substantive gap. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Python Architect] (blocking-severity) Port
_MAX_ENTRIESand_MAX_UNCOMPRESSEDbomb guard fromunpacker.pylines 85-96 tolocal_bundle._extract_zip_bundle(); hoist both constants to module level for sharing --apm install <crafted.zip>is unguarded whileapm unpack <crafted.zip>is rejected cleanly; missing test on a secure-by-default surface inherits blocking weight; the fix is five lines. - [Doc Writer] Fix
releasing-from-any-ci.md: replace allbuild/*.tar.gzglobs (10+ instances across four CI provider sections) withbuild/*.zip; add a migration callout -- CI pipelines following the docs will silently produce zero release artifacts once--archivedefaults to.zip; confirmed at lines 24, 30-31, 99, 104, 123-129, 146-151, 174-180. - [Doc Writer] Update
pack-a-bundle.md,install.md,unpack.md, andcommands.mdto show.zipas primary and.tar.gzas legacy escape hatch -- five doc surfaces still describe.tar.gzas canonical, giving new users a wrong mental model on day one; converges findings from doc-writer, oss-growth-hacker, and devx-ux-expert. - [Test Coverage Expert] Add hermetic integration test
TestInstallLocalBundleE2E::test_install_local_bundle_from_zipintests/integration/test_install_local_bundle_e2e.pycovering detect ->_extract_zip_bundle-> install -> file-deployment -- outcome: missing on a portability-by-manifest surface; the primary distribution path in this PR has no integration-tier guardrail. - [DevX UX Expert] Replace the
--archive-format-without---archivewarning withclick.UsageErrorusingctx.get_parameter_source('archive_format') != ParameterSource.DEFAULT; fold the action-hint into the error message; add a test intest_pack_cli_surface.py-- current warning fires only fortar.gz(not explicitzip), inconsistent with APM flag-misuse pattern.
Architecture
classDiagram
direction TB
class BuildOptions {
<<ValueObject>>
+bundle_archive bool
+bundle_archive_format str
}
class PackResult {
<<ValueObject>>
+bundle_path Path
+files list
}
class LocalBundleInfo {
<<ValueObject>>
+is_archive bool
+temp_dir Path
}
class packer {
<<Module>>
+pack_bundle(archive_format) PackResult
}
class plugin_exporter {
<<Module>>
+export_plugin_bundle(archive_format) PackResult
}
class unpacker {
<<Module>>
+unpack_bundle(bundle_path) UnpackResult
}
class local_bundle {
<<Module>>
+detect_local_bundle(path) LocalBundleInfo
-_extract_zip_bundle(path) LocalBundleInfo
}
class extractor {
<<Module>>
+extract_archive(data, digest, dest) str
-_safe_extract_zip(zf, dest)
}
class BuildOrchestrator {
<<Orchestrator>>
+run(options) BuildResult
}
BuildOrchestrator ..> BuildOptions : reads
BuildOrchestrator ..> packer : fmt=apm
BuildOrchestrator ..> plugin_exporter : fmt=plugin
packer ..> PackResult : produces
plugin_exporter ..> PackResult : produces
local_bundle ..> LocalBundleInfo : produces
class packer:::touched
class plugin_exporter:::touched
class unpacker:::touched
class local_bundle:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
P1["apm pack --format apm --archive"] --> W1["pack_bundle packer.py:277"]
P2["apm pack --format plugin --archive"] --> W2["export_plugin_bundle plugin_exporter.py:650"]
W1 --> WL["FS: identical rglob write-archive block\narchive_format==tar.gz: tarfile.open w:gz\nelse: ZipFile ZIP_DEFLATED + zf.write"]
W2 --> WL
I1["apm install local.zip"] --> R2["_extract_zip_bundle local_bundle.py:201"]
I2["apm unpack bundle.zip"] --> R1["unpack_bundle unpacker.py:59"]
I3["registry install path"] --> R3["_safe_extract_zip extractor.py:202"]
R1 --> RV1["validate path+symlink\nbomb guard: _MAX_ENTRIES=10k _MAX_UNCOMPRESSED=512MB\nunpacker.py:85-97"]
R2 --> RV2["validate path+symlink\nNO bomb guard\nlocal_bundle.py:212-230"]
R3 --> RV3["stream per-member via zf.open\nno extractall call\nextractor.py:209-233"]
RV1 --> EX1["FS: zf.extractall temp_dir"]
RV2 --> EX2["FS: zf.extractall temp_dir"]
style RV2 fill:#ffcccc,stroke:#cc0000
style WL fill:#fff3cc,stroke:#cc8800
Recommendation
Fix two items before merge: (1) port the ZIP bomb guard (_MAX_ENTRIES, _MAX_UNCOMPRESSED) from unpacker.py to local_bundle._extract_zip_bundle() -- five lines, eliminating an unguarded resource-exhaustion path on the new default distribution format; (2) update releasing-from-any-ci.md to replace build/*.tar.gz globs with build/*.zip across all four CI provider sections, preventing silent pipeline breakage for any team following the docs. Both fixes are bounded, reviewable in the same PR, and do not touch the core design. All remaining findings -- stale docs in pack-a-bundle.md, install.md, unpack.md, commands.md; UsageError vs warning; integration test for apm install bundle.zip; the pre-existing file_size accounting weakness in the bomb guard -- are queued as immediate follow-ups to land before the next release note. The core design is correct and the growth signal is real; two trivial fixes are all that stand between this PR and a clean merge.
Full per-persona findings
Python Architect
-
[blocking] ZIP bomb guard absent in
local_bundle._extract_zip_bundle()while present inunpacker.unpack_bundle()atsrc/apm_cli/bundle/local_bundle.py:201
Bothunpack_bundle()and_extract_zip_bundle()accept user-supplied.zipfiles and callzf.extractall()after per-member validation.unpacker.pyguards with_MAX_ENTRIES=10,000and_MAX_UNCOMPRESSED=512 MBat lines 85-97._extract_zip_bundle()applies identical path-traversal and symlink checks but has no bomb guard:apm install evil.zipwith 10,001 entries or >512 MB uncompressed will exhaust file descriptors or RAM duringextractall(), whileapm unpack evil.zipis rejected cleanly.
Suggested: Add the same guard immediately afterzf.infolist()in_extract_zip_bundle():if len(members) > _MAX_ZIP_ENTRIES or sum(m.file_size for m in members) > _MAX_ZIP_UNCOMPRESSED: shutil.rmtree(temp_dir, ignore_errors=True); return None. Hoist both constants to module level for sharing. -
[recommended] Archive-creation rglob block duplicated verbatim in
packer.pysection 10 andplugin_exporter.pysection 15 atsrc/apm_cli/bundle/packer.py:277
Both files contain byte-for-byte identical archive-creation logic:archive_formatbranch,ensure_path_within()guard,rglob('*')loop with symlink filter, andtarfile.add/zf.writewith the same arcname pattern. A future third format requires editing both files; a symlink-filter regression in one will not propagate to the other.
Suggested: Extract towrite_bundle_archive(bundle_dir, output_dir, name, archive_format) -> Pathhelper inbundle/packer.py. -
[recommended] Three independent ZIP member-validation implementations with diverging safety properties at
src/apm_cli/bundle/unpacker.py:65
extractor._safe_extract_zip()checks the full 16-bit Unix mode word;unpacker.pyandlocal_bundle.pycheck only the upper 4-bit type field. Path-traversal predicates are copy-pasted across files.
Suggested: Addvalidate_zip_member(member: zipfile.ZipInfo) -> Nonetoutils/path_security.pyand route all three paths through it. -
[nit]
_MAX_UNCOMPRESSEDand_MAX_ENTRIESdefined as block-local variables insideunpacker.pyif-branch atsrc/apm_cli/bundle/unpacker.py:85
Constants invisible to module-level tooling and cannot be referenced by callers or shared withlocal_bundle.py.
Suggested: Hoist to module level:_MAX_ZIP_UNCOMPRESSED = 512 * 1024 * 1024and_MAX_ZIP_ENTRIES = 10_000. -
[nit]
_looks_like_archive()only matches.tar.gz/.tgz-- misleading name atsrc/apm_cli/bundle/local_bundle.py:135
A future contributor scanning for archive-type detection will miss the.zipbranch dispatched separately.
Suggested: Rename to_looks_like_tarball().
CLI Logging Expert
-
[recommended] No-op warning missing an action hint at
src/apm_cli/commands/pack.py
APM output principle: every warning must include the fix."--archive-format tar.gz has no effect without --archive"leaves the user to infer the remedy.
Suggested:logger.warning(f"--archive-format {archive_format} has no effect without --archive -- add --archive to produce a .{archive_format} archive") -
[recommended] ZIP-bomb ValueError reports raw byte integers; human-readable units needed for CI operators at
src/apm_cli/bundle/unpacker.py
"629145600 bytes exceeds 536870912 bytes" requires mental arithmetic. CI operators need MB units to quickly assess severity.
Suggested:raise ValueError(f"ZIP archive uncompressed size {total_size // (1024 * 1024)} MB exceeds limit of {_MAX_UNCOMPRESSED // (1024 * 1024)} MB") -
[nit]
{archive_format!r}renders Python repr quotes in the warning string atsrc/apm_cli/commands/pack.py
Witharchive_format = "tar.gz",!rproduces'tar.gz'with single quotes in user output. Change to bare{archive_format}. -
[nit] Code comment overstates the warning scope at
src/apm_cli/commands/pack.py
Comment: "Warn when --archive-format is set but --archive is not -- the flag is a no-op." But condition isarchive_format != "zip", so explicit--archive-format zipwithout--archiveis silently swallowed. Better: "Warn when a non-default --archive-format is given without --archive." -
[nit] ZIP error says "symlink" while tar.gz branch says "symlink/hardlink" -- minor inconsistency at
src/apm_cli/bundle/unpacker.py
Align phrasing across archive format branches. ZIP has no hardlink concept so "symlink" is correct; make error message style consistent with sibling message.
DevX UX Expert
-
[recommended] Shipped skill prose for
apm install <BUNDLE-PATH>still lists only.tar.gz/.tgzatpackages/apm-guide/.apm/skills/apm-usage/commands.md
Agents loading this skill file at runtime will tell users only.tar.gzarchives are supported -- wrong guidance for the new.zipdefault on day one of merge.
Suggested: Change "or to a.tar.gz/.tgzarchive whose extracted root containsplugin.json" to "or to a.zip,.tar.gz, or.tgzarchive whose extracted root containsplugin.json". -
[recommended]
--archive-formatwithout--archiveshould raiseclick.UsageError, not emit a warning atsrc/apm_cli/commands/pack.py
Guard fires only fortar.gz(not explicitzip), producing inconsistent behavior. APM pattern isUsageErrorfor inapplicable flags (--aswithout a bundle path raisesUsageError). Package managers in the reference set (cargo, pip) error on inapplicable flags.
Suggested:if not archive and ctx.get_parameter_source('archive_format') != click.core.ParameterSource.DEFAULT: raise click.UsageError('--archive-format has no effect without --archive; add --archive to produce an archive.') -
[nit]
tar.gzas a period-containingclick.Choicestring may not complete cleanly in generated shell completions atsrc/apm_cli/commands/pack.py
Validateapm pack --archive-format <TAB>completes correctly in bash, zsh, and fish before shipping. Consider aliasing totgzwith help clarifying it means.tar.gzoutput if completions break. -
[nit]
--archivehelp text cross-references--archive-format, making a simple boolean flag paragraph busy atsrc/apm_cli/commands/pack.py
Suggested: "Produce an archive instead of a directory. Default format: .zip. See --archive-format."
Supply Chain Security Expert
-
[recommended] ZIP bomb guard absent from
local_bundle._extract_zip_bundle()-- asymmetry withunpacker.pyatsrc/apm_cli/bundle/local_bundle.py
apm install <user-supplied-zip>routes through_extract_zip_bundle(). No entry-count or size limit beforezf.extractall(). A crafted ZIP with 50,000 entries can exhaust temp-dir disk space while leaving no actionable error message.
Suggested: Port_MAX_ENTRIESand_MAX_UNCOMPRESSEDguard fromunpacker.pyimmediately beforezf.extractall()in_extract_zip_bundle().
Proof (missing at static): grep for bomb guard inlocal_bundle.pyreturns zero results; same check confirmed present inunpacker.pylines 85-97. -
[recommended] Bomb guard sums attacker-controlled
ZipInfo.file_size, not measured output bytes atsrc/apm_cli/bundle/unpacker.py
A crafted ZIP can declarefile_size=0for every entry while DEFLATE stream expands to gigabytes.sum(m.file_size)would be 0 -- passing the 512 MB guard -- whileextractall()writes unbounded content. APM-produced ZIPs have accurate metadata, butapm unpackandapm installboth accept arbitrary user-supplied archives.
Suggested: Stream extraction counting bytes written, or enforce RLIMIT_FSIZE on the temp_dir before callingextractall().
Proof (manual at static): Python docs: ZipInfo.file_size sourced from central directory record, not from decompression. Python's zipfile.ZipFile does not cross-check actual output size against this field. -
[nit] Entry-count and size bomb guard should precede member-iteration loop for early exit at
src/apm_cli/bundle/unpacker.py
A ZIP with 1,000,000 entries triggers 1,000,000validate_path_segments()calls before the 10,000-entry limit fires. Hoistmembers = zf.infolist()and the bomb guard immediately inside thewith zipfile.ZipFile(...)block before the for-member validation loop. -
[nit] No unit tests cover ZIP bomb guard thresholds at
tests/unit/test_unpacker.py
TestUnpackZipSecuritytests path traversal and symlinks but not_MAX_ENTRIES/_MAX_UNCOMPRESSED. If either constant is removed, no test regresses.
Proof (missing at unit):tests/unit/test_unpacker.py-- proves: bomb guard constants have regression coverage -
[nit] Windows reserved device names (COM1, NUL, CON) pass all path guards at
src/apm_cli/bundle/unpacker.py
PureWindowsPath('COM1').drive == ''(falsy), so device names are not caught. Not a traversal bypass but a platform-specific DoS vector on Windows installations.
Suggested: Add segment-level check:re.compile(r'^(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])(\\..*)?$', re.IGNORECASE)applied to each path segment.
OSS Growth Hacker
-
[recommended]
apm unpackdocs still document.tar.gzexclusively despite this PR adding.zipextraction support atdocs/src/content/docs/reference/cli/unpack.md
Description says "extracts a .tar.gz archive". The PR adds.zipsupport tounpack_bundle()but the docs don't reflect it. New producers whoapm pack --archiveand share a.zipwill see docs that describe only.tar.gz.
Suggested: Update Description and first example to show.zipas primary,.tar.gzas legacy. -
[recommended]
install.mdandcommands.mdstill cite./bundle.tar.gzas canonical packed-bundle example atdocs/src/content/docs/reference/cli/install.md
install.mdPACKAGE_REF says "packed bundles (./bundle.tar.gz)".commands.mdlocal-bundle description still names only.tar.gz/.tgz. New users have the wrong extension for a bundle newly produced byapm pack --archive.
Suggested: Updateinstall.mdto list.zipfirst; updatecommands.mdparagraph. -
[nit] CHANGELOG Changed entry lacks a CI migration one-liner for pipeline maintainers at
CHANGELOG.md
CI pipelines that glob*.tar.gzartifacts will silently break. Append to the bullet: "CI migration: update artifact globs frombuild/*.tar.gztobuild/*.zip, or add--archive-format tar.gzto yourapm packinvocation to preserve the previous default." -
[nit]
publish.mdstill documents.tar.gzas auto-pack format (pre-existing drift from feat(publish): switch publish command archive format from tar.gz to zip #1695) atdocs/src/content/docs/reference/cli/publish.md
CHANGELOG references format alignment withapm publish, butpublish.mdstill describes.tar.gzoutput. Recommend a companion doc-fix PR before the next release note.
Auth Expert -- inactive
No auth-specific files changed. PR is a pure archive-format change with no impact on AuthResolver, token management, credential resolution, or remote-host auth semantics.
Doc Writer
-
[recommended]
producer/pack-a-bundle.mdstill describes.tar.gzas the archive output in three places atdocs/src/content/docs/producer/pack-a-bundle.md
Primary producer guide says "Add --archive to get a single .tar.gz", shows.tar.gzinline comment and a distribution example. New users following this guide will look for a file that no longer exists by default. -
[recommended]
releasing-from-any-ci.mdglobsbuild/*.tar.gzacross all four CI examples -- those globs match nothing once--archiveproduces.zipby default atdocs/src/content/docs/producer/releasing-from-any-ci.md
CI pipeline examples usebuild/*.tar.gzandbuild/*.tar.gz.sha256for sha256 loop and release-create upload. Globs expand to nothing once the default is.zip. Release artifacts silently omitted.
Suggested: Change all globs tobuild/*.zip; add--archiveto theapm packcall; add a migration callout in the section header. -
[recommended]
install.mddescription still references./bundle.tar.gzas the packed bundle format atdocs/src/content/docs/reference/cli/install.md
PACKAGE_REF description says "packed bundles (./bundle.tar.gz)". Install examples show only.tar.gzfor local bundle with--as. -
[recommended]
unpack.mddescription and all examples reference only.tar.gzatdocs/src/content/docs/reference/cli/unpack.md
BUNDLE_PATH description says "accepts either a .tar.gz archive". All five code examples usebundle.tar.gz. The CHANGELOG states.zipis now supported but docs don't reflect it. -
[recommended]
ci-cd.mdupdates file patterns without a migration callout for existing pipelines atdocs/src/content/docs/integrations/ci-cd.md
The "Pack & Distribute" section silently changes the glob without noting this is a change from previous behavior. Teams with existing pipelines hardcoding*.tar.gzpatterns get no warning.
Suggested: Add a :::note migration callout in the "Pack in CI" subsection. -
[nit] CHANGELOG places
--archive-formatunder### Changedinstead of### AddedatCHANGELOG.md
Keep a Changelog convention: new flags go in Added. The bullet even starts with "Add". -
[nit] CHANGELOG references
apm publishformat alignment butpublish.mdstill documents.tar.gzatCHANGELOG.md
Drop theapm publishcross-reference or open a companion task to updatepublish.mdfirst. -
[nit]
producer/index.mdstep 4 description still says.tar.gzatdocs/src/content/docs/producer/index.md
Suggested: Change to "apm pack produces a .zip bundle you can ship offline or to a marketplace". -
[nit]
concepts/glossary.mdbundle definition only mentions.tar.gzatdocs/src/content/docs/concepts/glossary.md
Suggested: "Either a directory or a.zip(default) or.tar.gzarchive containingplugin.jsonat the root." -
[nit]
enterprise/security.mdlocal bundle trust model says.tar.gzonly atdocs/src/content/docs/enterprise/security.md
Suggested: "apm install<bundle>accepts a directory,.zip, or.tar.gzproduced byapm pack."
Test Coverage Expert
-
[recommended] No hermetic integration test for
apm install <bundle.zip>-- the new default archive format attests/integration/test_install_local_bundle_e2e.py
test_install_local_bundle_e2e.pytests directory and tarball installs but has no zip test. The full pathdetect_local_bundle(.zip)->_extract_zip_bundle()->install_local_bundle()-> file deployment is untested at integration tier. A regression in the primary distribution path would be invisible until a user reports it.
Proof (missing at integration-with-fixtures):tests/integration/test_install_local_bundle_e2e.py::TestInstallLocalBundleE2E::test_install_local_bundle_from_zip-- proves: apm install (bundle.zip) deploys plugin bundle files end-to-end [devx,portability-by-manifest] -
[recommended] ZIP bomb guard in
unpacker.pyhas no regression trap attests/unit/test_unpacker.py
_MAX_ENTRIES=10,000and_MAX_UNCOMPRESSED=512 MBhave no test. If either constant is changed or the guard removed, no test catches the regression.
Proof (missing at unit):tests/unit/test_unpacker.py::TestUnpackZipSecurity::test_zip_bomb_entry_count_rejected-- proves: apm install (bloated.zip) is rejected before extraction begins [secure-by-default] -
[recommended]
TestUnpackZipSecurityis unit tier; path-segment validation on a security surface warrants an integration-with-fixtures CLI-level test attests/unit/test_unpacker.py
Unit tests exerciseunpack_bundle()directly with real zip files -- genuine protection. But no CLI-level test assertsapm install <traversal.zip>exits non-zero with a user-readable error message.
Proof (passed at unit):tests/unit/test_unpacker.py::TestUnpackZipSecurity::test_zip_path_traversal_rejected-- proves: unpack_bundle() rejects ZIP path-traversal entries at function level [secure-by-default] -
[nit]
--archive-formatwithout--archiveemits a warning but no test asserts the warning fires attests/unit/commands/test_pack_cli_surface.py
Proof (missing at unit):tests/unit/commands/test_pack_cli_surface.py::TestPackCmdFlags::test_archive_format_without_archive_emits_warning-- proves: --archive-format tar.gz without --archive shows the user a clear warning
Performance Expert
-
[recommended] Missing ZIP bomb guard in
_extract_zip_bundle()-- asymmetric withunpacker.pyatsrc/apm_cli/bundle/local_bundle.py
_extract_zip_bundle()callszf.extractall()with no entry-count or size limit. Disk-exhaustion from crafted ZIP viaapm install ./crafted.zipon the new primary distribution path.
Suggested: Port the five-line guard fromunpacker.py:85-97to_extract_zip_bundle(). -
[nit] Two separate
zf.infolist()calls inunpacker.py-- could be a single merged pass atsrc/apm_cli/bundle/unpacker.py
CPython'sinfolist()returnsself.filelistdirectly; two O(N) passes over the same list is sub-0.1ms for any realistic bundle. Aesthetic tightening only. -
[nit]
sorted(bundle_dir.rglob('*'))eagerly materializes all paths before filtering atsrc/apm_cli/bundle/packer.py
Negligible at bundle sizes (<200 files). Positive side-effect: deterministic archive entry order. No action required. -
[nit] No
compresslevelonZIP_DEFLATED-- default level 6 is appropriate but undocumented atsrc/apm_cli/bundle/packer.py
Suggested: Add comment:# level 6 (zlib default) -- sufficient for agent bundle sizes < 2MB.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pypi.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #1720 · sonnet46 22.7M · ◷
|
All findings addressed in da6d293. ZIP bomb guard scope (blocking) The guard was present in unpacker.py (the apm unpack path) but missing entirely from local_bundle._extract_zip_bundle() (the apm install ./bundle.zip path). Fixed: the same guard now applies in both paths. I also hoisted _MAX_ZIP_ENTRIES and _MAX_ZIP_UNCOMPRESSED to module level in both files so the thresholds are visible and easy to adjust, moved the check before the per-member validation loop (previously a million-entry ZIP still triggered a million validate_path_segments calls before being rejected), and updated the error messages to show sizes in MB rather than raw bytes. _looks_like_archive rename Renamed to _looks_like_tarball -- the old name was accurate for the original code but became actively misleading once .zip became the primary format. All three call sites updated. --archive-format without --archive Changed from a logger.warning (which only fired when the value was non-default) to a click.UsageError that fires whenever the flag is explicitly set on the command line without --archive, using ctx.get_parameter_source() to distinguish an explicit value from the default. The error message includes an action hint. releasing-from-any-ci.md CI globs All build/.tar.gz and build/.tar.gz.sha256 patterns replaced with build/.zip / build/.zip.sha256 across the canonical sequence and all four CI provider sections (GitHub Actions, GitLab CI, Jenkins, Azure DevOps). Remaining doc drift Updated pack-a-bundle.md, install.md, unpack.md, producer/index.md, glossary.md, security.md, and the commands.md skill file. The apm unpack reference preserves the mention of legacy .tar.gz support since that command is specifically the deploy path for --format apm tarballs. Tests Added two bomb guard threshold tests to TestUnpackZipSecurity (using monkeypatch to lower the limits so the tests are fast and deterministic) and two --archive-format CLI surface tests to TestPackCmdFlags. |
d209679 to
99a956b
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 3 | 2 | Format migration is sound; ZIP extraction logic triplicated across three modules with inconsistent strategies; archive creation loop duplicated verbatim in packer.py and plugin_exporter.py; security rejection in _extract_zip_bundle silently swallowed as None. |
| CLI Logging Expert | 0 | 1 | 1 | Live-run messages are correctly dynamic via bundle_path; dry-run omits archive extension so 'Would pack -> build/pkg-1.0.0' misrepresents the .zip output; two consumer docs still lead with .tar.gz. |
| DevX UX Expert | 0 | 2 | 2 | Solid escape-hatch design and clean UsageError; two recommended gaps: no at-pack-time size hint and --archive help lacks explicit migration signal for existing .tar.gz automation. |
| Supply Chain Security Expert | 0 | 1 | 1 | ZIP bomb guard uses attacker-controllable file_size metadata; guard is bypassable. All other extraction, creation, and Content-Type surfaces look sound. |
| OSS Growth Hacker | 0 | 2 | 3 | ZIP-by-default is the right growth call; Claude Code alignment and Windows unlock are real wins; one silent-break vector for upgrading CI pipelines and a narrative misstep in CHANGELOG need attention. |
| Doc Writer | 0 | 2 | 3 | Core reference docs (pack.md, pack-a-bundle.md, ci-cd.md, CHANGELOG) are accurate; two consumer-side pages and a CHANGELOG convention gap need follow-up. |
| Test Coverage Expert | 0 | 2 | 0 | Two gaps: _extract_zip_bundle security guards in local_bundle.py lack unit tests; no credential-free integration test for apm install bundle.zip. |
| Performance Expert | 0 | 1 | 3 | ZIP default adds 30-40%+ bytes to every cold registry install (Fetch-phase regression); no blocking regressions; three nits on extraction and pack paths. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[Test Coverage Expert] Add TestDetectZipBundleSecurity unit tests in test_local_bundle.py covering path-traversal, absolute-path, symlink, entry-count bomb, and size-bomb rejection via detect_local_bundle() -- _extract_zip_bundle() security guards have zero automated coverage in test_local_bundle.py; TestUnpackZipSecurity monkeypatches a different module. A drift in the checks would be invisible to CI.
-
[Test Coverage Expert] Add TestInstallLocalBundleE2E::test_install_local_bundle_from_zip credential-free integration test matching the existing tar.gz counterpart -- ZIP is now the default archive format; the primary apm install (archive) path has no integration-with-fixtures coverage without a live GitHub token.
-
[Supply Chain Security Expert] Replace metadata-based ZIP bomb size check (file_size sum) with a per-entry streaming byte counter via zf.open(); consolidate into utils/archive.py alongside the streaming pattern from extractor.py._safe_extract_zip -- ZipInfo.file_size is attacker-controllable; an adversary bypasses the 512 MB guard by declaring file_size=0 on all entries while the actual deflated payload expands to multiple GB. Convergent with python-architect duplication finding.
-
[Doc Writer] Add BREAKING: prefix to the CHANGELOG Changed entry, reorder to lead with the benefit sentence before the size-disclosure note, and update consumer/deploy-a-bundle.md to show .zip as the primary format -- omitting BREAKING: is inconsistent with the 0.19.0 apm publish precedent; consumer/deploy-a-bundle.md is un-updated (convergent doc-writer + cli-logging finding).
-
[CLI Logging Expert] Fix dry-run bundle_path to include the projected archive extension and add an explicit 'changed from .tar.gz' migration cue to the --archive help string -- dry-run output 'build/pkg-1.0.0' misrepresents what the real run produces ('build/pkg-1.0.0.zip'); CI authors have no in-CLI signal that the default changed (convergent cli-logging + devx-ux finding).
Architecture
classDiagram
direction TB
class commands_pack {
<<EntryPoint>>
+pack_cmd(archive, archive_format)
+UsageError guard: format without archive
}
class BuildOptions {
<<ValueObject>>
+bundle_archive: bool
+bundle_archive_format: str = "zip"
+bundle_format: str
}
class BuildOrchestrator {
<<Facade>>
+run(options) BuildResult
}
class packer {
<<Module>>
+pack_bundle(archive_format) PackResult
-_write_zip_inline() DUPLICATED
-_write_tar_inline() DUPLICATED
}
class plugin_exporter {
<<Module>>
+export_plugin_bundle(archive_format) PackResult
-_write_zip_inline() DUPLICATED
-_write_tar_inline() DUPLICATED
}
class unpacker {
<<Module>>
+unpack_bundle(bundle_path) UnpackResult
-validate_zip_members_inline()
-zf_extractall_noqa_S202()
}
class local_bundle {
<<Module>>
+_extract_zip_bundle(path) InfoOrNone
+detect_local_bundle(path) InfoOrNone
-validate_zip_members_inline() DUPLICATED
-zf_extractall_noqa_S202() DUPLICATED
-_MAX_ZIP_ENTRIES DUPLICATED
}
class registry_extractor {
<<Module>>
+_safe_extract_zip(zf, dest_root)
+extract_zip(data, digest, dest_root)
+extract_archive(data, digest, dest_root)
}
class path_security {
<<Module>>
+validate_path_segments(path_str)
+ensure_path_within(path, base)
}
class ArchiveUtils_GAP {
<<MISSING: utils/archive.py>>
+safe_extract_zip(zf, dest_root)
+write_zip_archive(bundle_dir, archive_path)
+MAX_ZIP_ENTRIES = 10000
+MAX_ZIP_UNCOMPRESSED = 512MB
}
commands_pack ..> BuildOptions : constructs
commands_pack ..> BuildOrchestrator : invokes
BuildOrchestrator ..> packer : delegates fmt!=plugin
BuildOrchestrator ..> plugin_exporter : delegates fmt==plugin
unpacker ..> path_security : validate_path_segments
local_bundle ..> path_security : validate_path_segments
registry_extractor ..> path_security : validate_path_segments
ArchiveUtils_GAP ..> path_security : would delegate
class commands_pack:::touched
class BuildOptions:::touched
class packer:::touched
class plugin_exporter:::touched
class unpacker:::touched
class local_bundle:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
CLI_P["apm pack --archive --archive-format zip|tar.gz
pack_cmd() commands/pack.py:277"]
G1{"archive_format set
without --archive?"}
E1["UsageError raised"]
BOPTS["BuildOptions bundle_archive_format=format"]
ORCH["BuildOrchestrator.run()"]
G2{"fmt == plugin?"}
PE["export_plugin_bundle(archive_format)
plugin_exporter.py:409"]
PB["pack_bundle(archive_format)
packer.py:27"]
G3{"archive_format?"}
G4{"archive_format?"}
Z1["ZipFile.write rglob
plugin_exporter.py:666"]
T1["tarfile.add rglob
plugin_exporter.py:658"]
Z2["ZipFile.write rglob
packer.py:293 -- DUPLICATE of Z1"]
T2["tarfile.add rglob
packer.py:285 -- DUPLICATE of T1"]
CLI_P --> G1
G1 -->|yes| E1
G1 -->|no| BOPTS --> ORCH --> G2
G2 -->|plugin| PE --> G3
G2 -->|apm| PB --> G4
G3 -->|zip| Z1
G3 -->|tar.gz| T1
G4 -->|zip| Z2
G4 -->|tar.gz| T2
CLI_U["apm unpack bundle.zip
unpack_bundle() unpacker.py:32"]
BG1["ZIP bomb guard
_MAX_ZIP_ENTRIES=10000"]
VL1["validate_path_segments + external_attr
per member"]
EX1["zf.extractall temp_dir
noqa S202"]
CP1["verify lockfile + copy to output_dir"]
CLI_U --> BG1 --> VL1 --> EX1 --> CP1
CLI_I["apm install bundle.zip
detect_local_bundle() local_bundle.py:252"]
EZB["_extract_zip_bundle()
local_bundle.py:204"]
BG2["ZIP bomb guard
_MAX_ZIP_ENTRIES DUPLICATED"]
VL2["validate_path_segments + external_attr
LOGIC DUPLICATED"]
EX2["zf.extractall temp_dir
DUPLICATED -- returns None on security rejection"]
GR{"bundle_root found?"}
SN["return None
Security reason silently lost"]
OK["LocalBundleInfo returned"]
CLI_I --> EZB --> BG2 --> VL2 --> EX2 --> GR
GR -->|no| SN
GR -->|yes| OK
SAFE["registry_extractor._safe_extract_zip
Hardened streaming per-entry
NOT reused by unpacker or local_bundle"]
SAFE -. should be shared util .-> EX1
SAFE -. should be shared util .-> EX2
Recommendation
Approve with followups. The ZIP-by-default switch is architecturally sound, strategically correct, and passes all panelist correctness checks. Items 1 and 2 (missing security extraction tests and missing ZIP install integration test) should be filed as issues and resolved before the next patch release -- they represent security-path automation gaps on the new primary format with no automated regression trap. Item 3 (file_size metadata bypass) should be resolved in a near-term patch; it is not a day-zero exploit risk given the entry-count guard, but it degrades the security guarantee that the guards imply. Items 4 and 5 (CHANGELOG BREAKING: prefix and dry-run/help-text migration signal) are low-effort and should be folded into this PR or a same-day follow-up commit to prevent CI breakage for upgrading users.
Full per-persona findings
Python Architect
-
[recommended] ZIP extraction logic triplicated across extractor.py, unpacker.py, and local_bundle.py with inconsistent extraction strategies at
src/apm_cli/bundle/unpacker.py:100
extractor.py._safe_extract_zip() streams each entry via zf.open(info) to an explicitly resolved destination path -- a hardened approach that never calls extractall(). Both new call sites (unpacker.py:100 and local_bundle.py:241) instead validate members inline then call zf.extractall(temp_dir) with noqa S202. This creates two distinct security tiers for nominally identical operations. Any future hardening must be applied in three places.
Suggested: Extract safe_extract_zip(zf, dest_root) into src/apm_cli/utils/archive.py using the streaming pattern from extractor.py._safe_extract_zip(). Import from unpacker.py and local_bundle.py. Relocate _MAX_ZIP_ENTRIES and _MAX_ZIP_UNCOMPRESSED to the same module. -
[recommended] ZIP and tar.gz creation loops duplicated verbatim in packer.py and plugin_exporter.py at
src/apm_cli/bundle/packer.py:293
The 8-line ZipFile.write block appears identically at packer.py:293-299 and plugin_exporter.py:666-672. The 4-line tarfile.add block is similarly duplicated. Any compression-level or arcname change must be applied to both.
Suggested: Add _write_zip_archive(bundle_dir, archive_path) and _write_tar_archive(bundle_dir, archive_path) to src/apm_cli/utils/archive.py. Replace both inline blocks with calls to these helpers. -
[recommended] _extract_zip_bundle() returns None on security violations, silently losing the rejection reason at
src/apm_cli/bundle/local_bundle.py:218
When a ZIP bomb limit or path-traversal entry is detected, _extract_zip_bundle() returns None. detect_local_bundle() returns None in turn, and the install command surfaces a generic 'not a local bundle' error. In contrast, unpack_bundle() raises ValueError with a specific message. A user hitting the entry limit gets a confusing error rather than a clear security gate message.
Suggested: Raise ValueError (or a new LocalBundleSecurityError) from _extract_zip_bundle() instead of returning None for security violations. Reserve None for 'not a ZIP' or 'no plugin.json found'. -
[nit] _MAX_ZIP_ENTRIES and _MAX_ZIP_UNCOMPRESSED constants defined twice in two separate modules at
src/apm_cli/bundle/local_bundle.py:46
_MAX_ZIP_ENTRIES = 10_000 and _MAX_ZIP_UNCOMPRESSED = 512 * 1024 * 1024 appear at unpacker.py:14-15 and local_bundle.py:46-47. A limit change requires two edits.
Suggested: Define once in utils/archive.py and import in both modules. -
[nit] archive_format validity guard duplicated in packer.py and plugin_exporter.py; already enforced by click.Choice at the CLI layer at
src/apm_cli/bundle/packer.py:278
Both packer.py:278 and plugin_exporter.py:651 containif archive_format not in ("zip", "tar.gz"): raise ValueError. The valid-format set is defined in three places.
Suggested: Define SUPPORTED_ARCHIVE_FORMATS = frozenset({"zip", "tar.gz"}) in utils/archive.py and use a single _validate_archive_format(fmt) from both packer.py and plugin_exporter.py.
CLI Logging Expert
-
[recommended] Dry-run 'Would pack' line silently drops the archive extension: user sees build/pkg-1.0.0, gets build/pkg-1.0.0.zip at
src/apm_cli/bundle/packer.py
packer.py:196-203 and plugin_exporter.py:548-551 both return the raw bundle_dir as bundle_path in their dry-run early-return paths, ignoring archive=True and archive_format entirely. The live path correctly reassigns result.bundle_path = archive_path. A user running 'apm pack --archive --dry-run' cannot tell which file type will be produced.
Suggested: In each dry-run early-return, compute the projected archive path when archive=True: dry_path = (output_dir / f"{pkg_name}-{pkg_version}.zip") if (archive and archive_format == "zip") else (output_dir / f"{pkg_name}-{pkg_version}.tar.gz") if archive else bundle_dir. -
[nit] consumer/deploy-a-bundle.md and consumer/index.md still lead with .tar.gz as the primary bundle format
With .zip now the default output of 'apm pack --archive', these pages are misaligned with what a fresh pack produces. deploy-a-bundle.md lines 6, 12, 47-48 use .tar.gz; consumer/index.md line 21 says 'received a local .tar.gz bundle'.
Suggested: Update opening prose to 'a directory or .zip (or legacy .tar.gz)' and swap primary code examples to .zip filenames.
DevX UX Expert
-
[recommended] --archive help text gives no runtime migration cue to existing .tar.gz automation authors at
src/apm_cli/commands/pack.py
A user whose CI script ran 'apm pack --archive' and then 'tar xzf build/*.tar.gz' will get a silent missing-file error from tar, not from APM. The escape hatch is discoverable via --help but the help text does not signal the default changed.
Suggested: Change help string to: 'Produce a .zip archive instead of a directory (changed from .tar.gz in this release; use --archive-format tar.gz to restore the previous format for existing CI pipelines).' -
[recommended] ZIP size overhead (30-130% larger) is CHANGELOG-only; not surfaced anywhere in the CLI at pack time at
src/apm_cli/commands/pack.py
A user storing artifacts in size-limited CI storage has no at-runtime hint to try --archive-format tar.gz.
Suggested: Append to the --archive-format help string: "'tar.gz' is typically 30-130% smaller for text-heavy bundles and preserves the previous default for CI pipelines that rely on it." -
[nit] Success log line does not hint at the size flag
Suggested: Under the 'Share with: apm install ...' line, add: logger.verbose_detail('Tip: use --archive-format tar.gz for smaller archives on text-heavy bundles.') -
[nit] ci-cd.md switches to bare 'unzip -o build/*.zip -d ./' without noting the --archive-format tar.gz fallback at
docs/src/content/docs/integrations/ci-cd.md
Suggested: Add a comment: '# Migrating from .tar.gz? Add --archive-format tar.gz to the apm pack step above.'
Supply Chain Security Expert
-
[recommended] ZIP bomb size guard reads attacker-supplied file_size metadata; actual decompression quota is not enforced at
src/apm_cli/bundle/unpacker.py:75
Both unpacker.py:75 and local_bundle.py:220 compute sum(m.file_size for m in members) against the 512 MB limit. ZipInfo.file_size is a field in the ZIP central-directory header; an attacker declares file_size=0 while deflated content expands to multiple GB. The entry-count guard (10,000) does not bound real disk writes: a 10,000-entry bomb with 50 MB real output per entry yields 500 GB of disk writes while passing both checks.
Suggested: Enforce a real byte quota during extraction: extract members individually via zf.open(member) and accumulate bytes-read with a running counter, raising ValueError when the running total exceeds _MAX_ZIP_UNCOMPRESSED. -
[nit] noqa: S202 suppression on extractall in unpacker.py lacks inline rationale at
src/apm_cli/bundle/unpacker.py:100
local_bundle.py:241 uses '# noqa: S202 -- validated above' which documents why the suppression is safe. unpacker.py:100 uses '# noqa: S202' with no explanation.
Suggested: Change to:zf.extractall(temp_dir) # noqa: S202 -- validated above (entry count, size, path traversal, symlink checks)
OSS Growth Hacker
-
[recommended] No runtime migration advisory emitted when apm pack --archive now produces .zip instead of .tar.gz at
src/apm_cli/commands/pack.py
Users who upgrade APM and run apm pack --archive in CI will silently get a .zip. Any downstream step globbing build/*.tar.gz matches nothing and may appear to succeed. CHANGELOG discloses the change, but pip-upgrade CI jobs do not read CHANGELOG.
Suggested: In the pack success path, when archive=True and archive_format='zip', emit a logger.info() line: 'Note: --archive now produces .zip by default (changed in this release). Use --archive-format tar.gz to restore the previous format for legacy pipelines.' -
[recommended] CHANGELOG 'Changed' entry buries the Claude Code hook inside the size-penalty sentence at
CHANGELOG.md
The entry immediately follows the benefit with '30-130% larger'. A skim-reader absorbs 'larger' as the salient fact.
Suggested: Lead with the benefit ('apm pack --archive now produces .zip, the format Claude Code and plugin hosts natively consume -- no conversion step required, natively extractable on Windows without WSL or tar.'), then a separate 'Note:' bullet for the size disclosure. -
[nit] producer/index.md step 4 description lost format specificity -- 'bundle' is opaque to a new user at
docs/src/content/docs/producer/index.md
Suggested: Change to: 'apm pack produces a .zip you can ship offline or to a marketplace' -
[nit] Windows native-extraction story is underplayed at
docs/src/content/docs/producer/pack-a-bundle.md
Suggested: Add callout near the --archive flag: 'ZIP archives are natively extractable on Windows -- no WSL, tar, or additional tooling required.' -
[nit] Pack success output does not report output file size at
src/apm_cli/commands/pack.py
Suggested: Include the output path and file size in the pack success message when --archive is set.
Doc Writer
-
[recommended] consumer/deploy-a-bundle.md not updated: intro and examples still show .tar.gz as the primary bundle format at
docs/src/content/docs/consumer/deploy-a-bundle.md:6
The page opens 'a directory or .tar.gz someone handed you' (line 6); first code example uses apm install ./dist/my-pkg-1.0.0.tar.gz (line 12). Cross-page inconsistency will confuse producers handing ZIP bundles to consumers.
Suggested: Line 6: 'a directory or .zip someone handed you (or a legacy .tar.gz)'. Line 12 example: use .zip filename. Line 22: 'zip archive (or legacy gzipped tarball)'. Lines 47-48 output block: show .zip path. -
[recommended] CHANGELOG ### Changed entry missing BREAKING: prefix, inconsistent with 0.19.0 apm publish precedent at
CHANGELOG.md
The 0.19.0 release used 'BREAKING:' for the apm publish format change to .zip. The apm pack --archive default change is equally pipeline-breaking.
Suggested: Prefix the entry: 'BREAKING:apm pack --archivenow produces.zipby default instead of.tar.gz.' -
[nit] consumer/index.md routing table row still says '.tar.gz bundle' at
docs/src/content/docs/consumer/index.md:21
Suggested: Change to 'You received a local .zip or .tar.gz bundle and need to install it' -
[nit] CHANGELOG ### Added has an orphan blank line before the --archive-format entry, creating a loose list at
CHANGELOG.md
Suggested: Remove the blank line between the enterprise bootstrap mirror entry and the --archive-format entry in ### Added. -
[nit] releasing-from-any-ci.md reference note says 'tarballs' for what the updated pipeline produces as .zip files at
docs/src/content/docs/producer/releasing-from-any-ci.md:89
Suggested: Change 'per-plugin tarballs' to 'per-plugin bundles'
Test Coverage Expert
-
[recommended] _extract_zip_bundle() security guards in local_bundle.py are uncovered; TestUnpackZipSecurity monkeypatches a different module's constants at
tests/unit/bundle/test_local_bundle.py
local_bundle.py defines its own _MAX_ZIP_ENTRIES=10_000 and _MAX_ZIP_UNCOMPRESSED=512MB (lines 46-47). _extract_zip_bundle() uses these guards plus path-traversal, absolute-path, and symlink rejection. TestUnpackZipSecurity monkeypatches apm_cli.bundle.unpacker._MAX_ZIP_ENTRIES and calls unpack_bundle() -- a separate code path. test_local_bundle.py has zero zip, ZipFile, or _MAX_ZIP references. A drift in the checks would be invisible to CI.
Suggested: Add TestDetectZipBundleSecurity calling detect_local_bundle() with crafted zip fixtures: test_detect_zip_path_traversal_rejected, test_detect_zip_absolute_path_rejected, test_detect_zip_symlink_rejected, test_detect_zip_bomb_entries_rejected (monkeypatching apm_cli.bundle.local_bundle._MAX_ZIP_ENTRIES), test_detect_zip_bomb_size_rejected.
Proof (missing at unit):tests/unit/bundle/test_local_bundle.py::TestDetectZipBundleSecurity::test_detect_zip_path_traversal_rejected-- proves: apm install bundle.zip rejects a malicious zip containing path-traversal entries without extracting any payload [secure-by-default] -
[recommended] No credential-free integration test for apm install bundle.zip; the only zip install coverage requires a live GITHUB_APM_PAT at
tests/integration/test_install_local_bundle_e2e.py
test_install_local_bundle_e2e.py has test_install_local_bundle_from_tarball for .tar.gz but no zip equivalent. test_pack_unpack_e2e.py::test_full_round_trip covers zip but is gated by pytest.mark.requires_github_token. Per the tier matrix, local bundle install pipeline floor is integration-with-fixtures. ZIP is now the default format.
Suggested: Add TestInstallLocalBundleE2E::test_install_local_bundle_from_zip: build bundle with _make_plugin_bundle, zip it, call _invoke_install(project, str(zip_path), '--target', 'copilot', monkeypatch=monkeypatch), assert exit_code == 0 and deployed files exist. No network call required.
Proof (missing at integration-with-fixtures):tests/integration/test_install_local_bundle_e2e.py::TestInstallLocalBundleE2E::test_install_local_bundle_from_zip-- proves: apm install bundle.zip deploys plugin files correctly end-to-end without a live GitHub token [portability-by-manifest]
Performance Expert
-
[recommended] ZIP default inflates registry download size 30-40%+ vs tar.gz on every cold install at
src/apm_cli/bundle/packer.py
Benchmark on a realistic 29-file, 162KB-raw skill bundle: tar.gz=28KB, zip=40KB (+40%). ZIP compresses each entry independently, losing cross-file string deduplication from gzip's sliding window. This is a deliberate design trade-off (Windows-native tooling vs compression density), not a bug.
Suggested: Add compresslevel=9 parameter to ZipFile() in packer.py:293 and plugin_exporter.py:666 to partially recoup the size gap, and surface --archive-format tar.gz in help text as the size-priority alternative. -
[nit] unpacker.py does two O(N) passes over ZIP members; extractor.py does one at
src/apm_cli/bundle/unpacker.py
unpacker.py lines 83-99 loop over all members for validation, then extractall() iterates members again. extractor.py._safe_extract_zip does a single pass: validate, then immediately stream bytes. For typical bundle sizes (<100 files) overhead is <5ms.
Suggested: Replace the validate-loop + extractall() two-pass with a single-pass pattern matching extractor.py._safe_extract_zip. -
[nit] sorted(bundle_dir.rglob('*')) eagerly materializes the full file list before archiving starts at
src/apm_cli/bundle/packer.py
Both packer.py and plugin_exporter.py force the generator to completion before any bytes are written. For typical APM bundles (<50 files) this is sub-millisecond.
Suggested: Non-blocking. No action required for this PR. -
[nit] download_archive() holds full archive bytes in memory via response.content at
src/apm_cli/deps/registry/client.py
With ZIP archives 30-40%+ larger, peak memory allocation during install grows proportionally. Pre-existing pattern unchanged by this PR.
Suggested: Pre-existing issue, not introduced by this PR. Future work: stream-hash on fetch.
Auth Expert -- inactive
No auth-surface files are touched: all changes are in bundle/ (packer, unpacker, local_bundle, plugin_exporter), commands/pack.py, commands/install.py (help text only), core/build_orchestrator.py, deps/registry/client.py (tarball_bytes->archive_bytes format rename, no auth headers), docs pages, and tests -- none of which involve token resolution, credential helpers, AuthResolver, host classification, or HTTP authorization flows.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1720 · sonnet46 20.9M · ◷
Replace tarfile/gzip with zipfile (ZIP_DEFLATED) in both pack_bundle (apm format) and export_plugin_bundle (plugin format). The unpacker gains .zip support as the primary path; .tar.gz extraction is kept for backward compatibility with existing bundles. Aligns apm pack --archive with apm publish, which switched to .zip in microsoft#1695, making the whole toolchain consistent on a single archive format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds --archive-format [zip|tar.gz] to apm pack --archive so callers can opt into .tar.gz output. Default remains zip. Threaded through BuildOptions → pack_bundle → export_plugin_bundle (both apm and plugin bundle formats).
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 2 | Facade consolidation in utils/archive.py is architecturally sound; OSError-silencing in _extract_zip_bundle and per-invocation migration notice are the two findings worth addressing. |
| CLI Logging Expert | 0 | 1 | 3 | Migration note fires on every zip pack with no suppression path and triggers even for explicit --archive-format zip; 3 nits on redundant prefixes and stale copy. |
| DevX UX Expert | 0 | 1 | 1 | Clean format-switch with good escape hatch; migration notice fires on every .zip pack (including explicit --archive-format zip), polluting CI logs permanently. |
| Supply Chain Security Expert | 0 | 1 | 2 | Core zip controls solid (path traversal, bomb cap, symlink reject, hash-before-extract). Tar.gz backward-compat branches skip the 512 MB bomb limit on Python <3.12. |
| OSS Growth Hacker | 0 | 1 | 2 | Strong ecosystem-alignment move with good migration UX; Windows story is under-surfaced and the release lacks a planned story beat for a genuinely shareworthy change. |
| Doc Writer | 0 | 3 | 2 | No blocking issues. Breaking change migration path is underdocumented: releasing-from-any-ci.md has no callout, ci-cd.md buries hint in code comment, reference/cli/pack.md uses release-scoped language. |
| Test Coverage Expert | 0 | 2 | 1 | Core zip default, CLI surface, and security guards are well covered; write_zip_archive symlink exclusion and tar.gz escape-hatch round-trip lack direct tests. |
| Performance Expert | 0 | 1 | 2 | compresslevel=9 costs +21% CPU for -0.2% size vs level 6 on real APM bundles; no blocking perf regressions; extraction consolidation is a net positive. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Import _extract_tar_gz_file from utils.archive in unpacker.py and local_bundle.py to close the tar.gz decompression-bomb gap on Python <3.12 -- The zip path caps total uncompressed bytes at 512 MB; the tar.gz backward-compat paths call tarfile.extractall() directly with no size limit on Python <3.12. A crafted .tar.gz escape-hatch bundle can exhaust disk or memory. The fix is already implemented in utils/archive.py as _extract_tar_gz_file() -- a two-line import swap, not a design change.
- [Test Coverage Expert] (blocking-severity) Add test_write_zip_archive_excludes_symlinks in tests/unit/utils/test_archive.py (evidence outcome: missing, principle: secure-by-default) -- write_zip_archive is a new public function on a secure-by-default surface. Packer-level tests do not inject a symlink into the assembled bundle_dir before calling write_zip_archive. Without this test a regression in the write path is CI-invisible.
- [CLI Logging Expert] Gate migration notice on ParameterSource.COMMANDLINE at pack.py L604; plan to drop it in the next release cycle; strip 'in this release' copy from help text in the same pass -- Three panelists independently diagnosed the same defect: the banner fires on every .zip pack with no suppression path, including when the user explicitly passed --archive-format zip. The fix is one conditional.
- [Doc Writer] Add Starlight breaking-change callouts in releasing-from-any-ci.md, ci-cd.md, and reference/cli/pack.md -- releasing-from-any-ci.md silently rewrote all .tar.gz references to .zip with no in-page migration notice; ci-cd.md buries the hint in a YAML code comment invisible to copy-paste users; pack.md uses release-scoped language in a permanent reference table. Teams upgrading from the prior default will have broken CI artifact globs with no diagnostic text.
- [Test Coverage Expert] Add test_install_local_bundle_from_pack_tar_gz integration test in tests/integration/ (evidence outcome: missing, principle: portability-by-manifest) -- No test chains pack_bundle(archive=True, archive_format='tar.gz') output through detect_local_bundle and install_local_bundle. If a regression broke the tar.gz escape hatch, only a user with a live CI pipeline would catch it.
Architecture
classDiagram
direction TB
class ArchiveUtils {
<<Facade>>
+SUPPORTED_ARCHIVE_FORMATS frozenset
+MAX_ZIP_ENTRIES int
+MAX_ZIP_UNCOMPRESSED int
+validate_archive_format(fmt) void
+projected_archive_path(dir, name, fmt) Path
+write_zip_archive(bundle_dir, archive_path) void
+write_tar_archive(bundle_dir, archive_path) void
+safe_extract_zip(zf, dest_root, error_type) list
+download_and_extract_archive(url, dest) list
}
note for ArchiveUtils "Facade: single source of truth for archive write+read+validate. Parameterized Adapter: safe_extract_zip(error_type=T) lets bundle callers raise ValueError while registry callers raise UnsafeTarballError."
class BuildOptions {
<<ValueObject>>
+bundle_archive bool
+bundle_archive_format str
+bundle_format str
+bundle_output Path
+dry_run bool
}
class PackResult {
<<ValueObject>>
+bundle_path Path
+files list
+lockfile_enriched bool
+mapped_count int
}
class PackerModule {
<<Module>>
+pack_bundle(root, outdir, archive, archive_format) PackResult
}
class PluginExporterModule {
<<Module>>
+export_plugin_bundle(root, outdir, archive, archive_format) PackResult
}
class UnpackerModule {
<<Module>>
+unpack_bundle(bundle_path, outdir) UnpackResult
}
class LocalBundleModule {
<<Module>>
+detect_local_bundle(path) LocalBundleInfo
-_extract_zip_bundle(path) LocalBundleInfo
-_looks_like_tarball(path) bool
}
class ExtractorModule {
<<Module>>
+extract_archive(data, digest, dest, content_type) str
}
class PackCmd {
<<Module>>
+pack_cmd(archive, archive_format)
-_bundle_size_suffix(path) str
}
PackCmd ..> BuildOptions : populates
PackCmd ..> PackerModule : via orchestrator
PackerModule ..> ArchiveUtils : validate + write + projected_path
PackerModule ..> PluginExporterModule : delegates plugin fmt
PackerModule ..> PackResult : produces
PluginExporterModule ..> ArchiveUtils : validate + write + projected_path
UnpackerModule ..> ArchiveUtils : safe_extract_zip
LocalBundleModule ..> ArchiveUtils : safe_extract_zip
ExtractorModule ..> ArchiveUtils : safe_extract_zip
class ArchiveUtils:::touched
class BuildOptions:::touched
class PackerModule:::touched
class PluginExporterModule:::touched
class UnpackerModule:::touched
class LocalBundleModule:::touched
class ExtractorModule:::touched
class PackCmd:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm pack --archive --archive-format fmt])
A --> B[pack_cmd / commands/pack.py]
B --> C{archive_format set without --archive?}
C -->|yes| D[/UsageError: add --archive/]
C -->|no| E[BuildOptions bundle_archive_format=zip]
E --> F[BundleProducer.produce / build_orchestrator.py]
F --> G[pack_bundle / bundle/packer.py]
G --> H{fmt?}
H -->|plugin| I[export_plugin_bundle / bundle/plugin_exporter.py]
H -->|apm default| J[FS: assemble bundle_dir + copy files]
I --> K[validate_archive_format + projected_archive_path / utils/archive.py]
J --> K
K --> L{archive_format?}
L -->|tar.gz| M[FS: write_tar_archive / tarfile.open w:gz / symlinks excluded]
L -->|zip default| N[FS: write_zip_archive / ZIP_DEFLATED level 9 / symlinks excluded]
M --> O[FS: shutil.rmtree bundle_dir]
N --> O
O --> P[PackResult.bundle_path = archive_path]
P --> Q[_render_bundle_result + _bundle_size_suffix / pack.py]
Q --> R([success + size suffix + migration note on EVERY invocation])
A2([apm install bundle.zip])
A2 --> B2[install / commands/install.py]
B2 --> C2[detect_local_bundle / bundle/local_bundle.py]
C2 --> D2{name.endswith .zip?}
D2 -->|yes| E2[_extract_zip_bundle / NEW primary path]
D2 -->|no| F2{_looks_like_tarball?}
F2 -->|yes| G2[tarfile.open r:gz / legacy compat]
E2 --> H2[safe_extract_zip / utils/archive.py / error_type=ValueError]
H2 -->|BadZipFile or OSError| I2([return None - NOTE: OSError silenced, see finding])
H2 -->|ValueError security violation| J2[/ValueError to UsageError / install.py/]
H2 -->|ok| K2[_find_extracted_root / locate plugin.json]
G2 --> L2[LocalBundleInfo is_archive=True]
K2 --> L2
L2 --> M2[install_local_bundle / install/local_bundle_handler.py]
Recommendation
Merge after the author closes two blocking items in the same PR: (1) the tar.gz decompression-bomb gap in unpacker.py and local_bundle.py (two-line import swap against the already-implemented _extract_tar_gz_file cap in utils/archive.py), and (2) the two missing evidence-anchored tests (test_write_zip_archive_excludes_symlinks and test_install_local_bundle_from_pack_tar_gz). Before merge, also gate the migration notice on ParameterSource.COMMANDLINE and add Starlight callouts to releasing-from-any-ci.md, ci-cd.md, and reference/cli/pack.md -- low effort for the author, high protection for existing .tar.gz users against silent CI breakage. If the author is already in archive.py for the bomb fix, fold in compresslevel=6 at zero incremental cost (performance-expert: +21% CPU overhead at level 9 for -0.2% size gain vs level 6). Surface the Windows-extraction story as a visual callout in pack-a-bundle.md before merge to capture the enterprise conversion moment this PR quietly unlocks.
Full per-persona findings
Python Architect
-
[recommended] OSError during zip extraction silently returns None, masking disk/permission failures as non-bundle at
src/apm_cli/bundle/local_bundle.py:223
In _extract_zip_bundle,except (zipfile.BadZipFile, OSError)catches both 'not a zip file' and raw I/O errors from safe_extract_zip's inner file writes. If the temp dir has a permission or disk-full condition, the function returns None instead of propagating -- making detect_local_bundle return None, which causes install.py to fall through to the registry clone path and emit a misleading error. The tar.gz path re-raises all errors after cleanup, making the new zip path asymmetric with the legacy path it sits next to.
Suggested: Separate 'not a zip' from 'write failed'. Catch only zipfile.BadZipFile for the return-None path; re-raise OSError after cleanup:except zipfile.BadZipFile: shutil.rmtree(temp_dir, ignore_errors=True); return Nonethenexcept (ValueError, OSError): shutil.rmtree(temp_dir, ignore_errors=True); raise -
[recommended] Migration notice fires on every .zip pack forever with no suppression path at
src/apm_cli/commands/pack.py:607
The info message fires unconditionally on every successful zip-format pack. In CI pipelines that have already migrated, it repeats every build forever with no way to suppress it.
Suggested: Gate on ParameterSource.COMMANDLINE: only show notice when archive_format was defaulted, not explicitly passed. Plan to drop after one release cycle. -
[nit] validate_archive_format called twice -- explicitly, then inside projected_archive_path at
src/apm_cli/bundle/packer.py:286
Double call is harmless but blurs ownership. Remove the explicit call before projected_archive_path() in both packer.py and plugin_exporter.py. -
[nit] _MAX_ZIP_ENTRIES/_MAX_ZIP_UNCOMPRESSED aliases in unpacker.py and local_bundle.py are pointless indirection at
src/apm_cli/bundle/unpacker.py:15
Import and immediately rebind under private names. Use MAX_ZIP_ENTRIES/MAX_ZIP_UNCOMPRESSED directly at the call sites.
CLI Logging Expert
-
[recommended] Migration note fires on every .zip pack forever with no way to suppress it, including when user explicitly passed --archive-format zip at
src/apm_cli/commands/pack.py:604
The guard at line 604 fires unconditionally. A user who already knows and intentionally chose zip sees the migration banner every run. CI pipelines see it every build forever.logger.infois not gated by--verbose.
Suggested: Computearchive_format_was_defaulted = ctx.get_parameter_source('archive_format') is not click.core.ParameterSource.COMMANDLINEand gate on it. -
[nit] Hardcoded 'in this release' will be stale copy in runtime message and --archive help text at
src/apm_cli/commands/pack.py:606
A relative temporal reference without a version anchor. Drop 'in this release' from both strings. -
[nit] Hand-rolled 'Note:' prefix on logger.info() is redundant with the [i] symbol at
src/apm_cli/commands/pack.py:605
CommandLogger.info() already renders with [i]. Drop the 'Note: ' prefix. -
[nit] verbose_detail size tip duplicates tar.gz guidance already in the info message above it at
src/apm_cli/commands/pack.py:609
In verbose mode two consecutive messages recommend --archive-format tar.gz. Merge into one coherent verbose block.
DevX UX Expert
-
[recommended] Migration notice fires unconditionally on every .zip output, even when --archive-format zip was explicitly passed at
src/apm_cli/commands/pack.py
A user who writesapm pack --archive --archive-format zipalready knows what they want; the migration notice is backwards noise. No TTL. Violates the 'quiet on the happy path' contract.
Suggested: Gate on ParameterSource.COMMANDLINE. Plan to drop the notice entirely in the following release. -
[nit] --archive help text embeds release-note copy that belongs in CHANGELOG, not --help at
src/apm_cli/commands/pack.py
Parenthetical '(changed from .tar.gz in this release)' ages poorly.
Suggested: Trim to:"Produce a .zip archive instead of a directory. Use --archive-format tar.gz for legacy CI pipelines."
Supply Chain Security Expert
-
[recommended] Tar.gz backward-compat extraction in unpack_bundle and detect_local_bundle has no decompression-bomb limit on Python <3.12 at
src/apm_cli/bundle/unpacker.py
The new .zip path correctly caps total uncompressed bytes at 512 MB via safe_extract_zip. The parallel .tar.gz backward-compat paths call tar.extractall() directly without a size cap when sys.version_info < (3, 12). A crafted .tar.gz bundle -- the exact format this PR retains for legacy CI -- can exhaust disk or memory with no limit. utils/archive.py already exposes _extract_tar_gz_file() with the size cap.
Suggested: Import _extract_tar_gz_file from utils.archive and replace the tarfile.open()+extractall() block in unpack_bundle and detect_local_bundle with _extract_tar_gz_file(). -
[nit] _zip_member_target does not explicitly reject null bytes in member names at
src/apm_cli/utils/archive.py
The tar path explicitly raises on '\x00' in member_path. Not exploitable but inconsistent with the tar path.
Suggested: Add early guard:if '\x00' in member_name: _raise(error_type, f'Refusing ZIP entry with null byte: {member_name!r}'). -
[nit] Unix-mode symlink check skipped when external_attr == 0 (ZIPs produced on Windows) at
src/apm_cli/utils/archive.py
Not exploitable -- Python's zipfile.open() never materialises symlinks from the data stream. Needs a comment explaining why the short-circuit is safe.
OSS Growth Hacker
-
[recommended] Windows-native extraction is the Why do we need a GitHub token? #1 new-user hook but is buried mid-paragraph in pack-a-bundle.md with no visual callout
Removing the WSL/tar requirement on Windows is a silent adoption unblocker for the enterprise Windows developer segment. Users who land on pack-a-bundle.md via search will miss this entirely.
Suggested: Add a visually distinct callout block at the top of the --archive section in docs/src/content/docs/producer/pack-a-bundle.md noting Windows native extractability. -
[nit] No story beat planned for a genuinely shareworthy change: Claude Code alignment + Windows unblocking in a single PR
This PR removes two real blockers simultaneously. The narrative shape is a mini-launch moment. Flag for the next release post. -
[nit] Reference deployment in releasing-from-any-ci.md may show .tar.gz until it upgrades, confusing readers
Teams clicking through the reference deployment will see .tar.gz and wonder if the doc or code is wrong.
Auth Expert -- inactive
No auth surfaces appear in the diff; all 37 changed files are confined to archive-format handling, and the zip-vs-tar.gz format switch introduces no credential, token, or host-resolution logic.
Doc Writer
-
[recommended] releasing-from-any-ci.md silently changed all *.tar.gz references to *.zip with no in-page migration note at
docs/src/content/docs/producer/releasing-from-any-ci.md
The canonical CI release-pipeline reference now outputs .zip with no explanation. Teams upgrading from a release that defaulted to .tar.gz will have broken CI pipelines (sha256sum, upload globs, artifact names) with no in-page text telling them why.
Suggested: Add a :::note[Breaking change: --archive now produces .zip]::: callout after the canonical sequence block. State the old default, new default, and escape hatch. -
[recommended] reference/cli/pack.md Options table embeds release-scoped language in the permanent --archive description at
docs/src/content/docs/reference/cli/pack.md
The parenthetical '(changed from .tar.gz)' is release-relative language -- stale noise in six months.
Suggested: Strip the parenthetical. Add a :::note[Migrating from .tar.gz]::: callout in the Behavior section. -
[recommended] ci-cd.md migration hint for a breaking change is buried inside a YAML code comment at
docs/src/content/docs/integrations/ci-cd.md
Code comments are easy to skim past and invisible to copy-paste users. A BREAKING CHANGE deserves a structured Starlight callout.
Suggested: Replace the inline code comment with a :::caution[Breaking change: default archive format changed to .zip]::: callout. -
[nit] CHANGELOG claims ZIP is 30-130% larger than tar.gz but this range has no backing benchmark at
CHANGELOG.md
The specific range implies measurement. Either remove the range or reference a benchmark. -
[nit] pack.md Behavior section does not document the runtime info note emitted on every .zip pack at
docs/src/content/docs/reference/cli/pack.md
The CLI emits an INFO message post-success that users reading the reference doc will not find documented.
Test Coverage Expert
-
[recommended] write_zip_archive / write_tar_archive symlink exclusion in utils/archive.py has no direct test at
src/apm_cli/utils/archive.py
Packer-level tests exercise symlink scanning upstream but do not inject a symlink into the assembled bundle_dir before calling write_zip_archive. Grep confirms zero hits for either function name in the test tree.
Proof (missing):tests/unit/utils/test_archive.py::test_write_zip_archive_excludes_symlinks-- proves: apm pack --archive does not include symlinks from the assembled bundle directory in the produced zip output [secure-by-default]
assert 'link.txt' not in zipfile.ZipFile(result).namelist() -
[recommended] tar.gz escape-hatch (--archive-format tar.gz) is not integration-tested as a pack->install round-trip at
src/apm_cli/bundle/packer.py
No test chains pack_bundle(archive=True, archive_format='tar.gz') output through detect_local_bundle and install_local_bundle. The e2e test only globs for *.zip.
Proof (missing):tests/integration/test_install_local_bundle_e2e.py::test_install_local_bundle_from_pack_tar_gz-- proves: apm install ./bundle.tar.gz works when the tarball was produced by apm pack --archive --archive-format tar.gz [portability-by-manifest,devx]
assert _invoke_install(consumer, str(result.bundle_path)).exit_code == 0 -
[nit] Corrupt zip -> None + temp-dir cleanup path in _extract_zip_bundle is not tested at
src/apm_cli/bundle/local_bundle.py
TestDetectZipBundleSecurity covers path-traversal, symlinks, bombs -- but not a corrupt (BadZipFile) payload.
Proof (missing):tests/unit/bundle/test_local_bundle.py::test_detect_zip_corrupt_returns_none_and_cleans_temp_dir
assert detect_local_bundle(corrupt_zip_path) is None
Performance Expert
-
[recommended] write_zip_archive uses compresslevel=9, adding ~21% CPU overhead for ~0.2% size reduction vs compresslevel=6 at
src/apm_cli/utils/archive.py
CPython 3.12 benchmarks on 35 real APM source files (219 KiB uncompressed): compresslevel=9 takes 11.70ms; compresslevel=6 takes 9.65ms. +2.05ms (+21% CPU) for -160 bytes (-0.2% size). Level 6 is the standard zlib inflection point where the compression curve flattens.
Suggested: Change compresslevel=9 to compresslevel=6 in write_zip_archive. For write_tar_archive, use tarfile.open(archive_path, 'w:gz', compresslevel=6). -
[nit] sorted(bundle_dir.rglob('*')) materializes the full path list before compression starts
0.67ms sort overhead for a 170-file bundle. Acceptable cost for reproducible archive ordering. No change recommended. -
[nit] safe_extract_zip chunk size upgrade from 64 KiB to 1 MiB is a net positive; deserves a comment documenting the deliberate choice.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1720 · sonnet46 20.9M · ◷
Co-authored-by: nadav-y <nadav-y@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 3 | Solid consolidation; archive.py correctly becomes the single write+extract authority. Three follow-ups: _raise needs -> NoReturn annotation, fragile message-string inspection in unpacker.py backward-compat path, eager requests.Session at module level. |
| CLI Logging Expert | 0 | 2 | 2 | Migration notice fires permanently with no expiry gate, and uses logger.info instead of logger.warning for a pipeline-breaking behavior change. |
| DevX UX Expert | 0 | 3 | 2 | Well-structured breaking-change rollout; flag surface is idiomatic. Three gaps: click.Choice case sensitivity, migration notice frequency, --archive-format help text length. |
| Supply Chain Security | 0 | 1 | 3 | Zip guards are layered correctly; one confirmed error_type API contract breach on null-byte filenames warrants a targeted fix before the download pipeline sees crafted archives. |
| OSS Growth Hacker | 0 | 2 | 3 | Strong execution on genuine ecosystem alignment. Windows-native extraction and Claude Code format matching are real adoption unlocks; CHANGELOG narrative ends on the wrong note. |
| Doc Writer | 0 | 2 | 4 | All 14 doc files updated; migration cautions placed in high-traffic pages; Windows tip is a genuine reader win. Two recommended: ci-cd.md caution wording, CHANGELOG cross-reference gap. |
| Test Coverage Expert | 0 | 2 | 1 | ZIP security rejection, CLI flag contract, and install round-trip well-covered. Two recommended gaps: CLI-tier UsageError wording for zip security failures untested; plugin_exporter.py tar.gz branch has only a dry-run test. |
| Performance Expert | 0 | 3 | 2 | sorted(rglob(*)) materializes full path list before first archive write; compresslevel=9 costs 2-3x CPU for <3% gain; ensure_path_within resolves dest_root per member. Positive: ZIP extract chunk grew from 64KB to 1MB. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] Add null-byte guard at top of _zip_member_target and unit test asserting error_type contract holds for null-byte zip entry filenames -- evidence.outcome=missing on a secure-by-default surface; null-byte entry reaches Path.resolve() before the _raise() wrapper, leaking raw ValueError to all ArchiveError callers. One-line fix:
if "\x00" in member_name: _raise(error_type, f"Refusing to extract path with null byte: {member_name!r}"). - [Test Coverage Expert] Add integration test: apm install evil.zip exits non-zero with click.UsageError("Bundle security check failed: ..."), not a raw traceback -- evidence.outcome=missing on a secure-by-default surface; the extraction guard and the install CLI boundary need independent regression traps. Without this, a refactor in either file silently reopens the path.
- [DevX UX Expert] Fix click.Choice to case_sensitive=False so --archive-format TAR.GZ does not silently fail -- silent failure on a user-typed two-value enum violates the pragmatic-as-npm promise and will generate confused-user tickets in the first adoption wave. One-character fix with zero behavioral risk.
- [CLI Logging Expert] Upgrade migration notice from logger.info to logger.warning and add a once-seen gate to suppress repeat emissions in CI -- two panelists independently flagged this; logger.info for a pipeline-breaking format change is semantically wrong and indefinite emission drives workarounds before the format switch has time to land.
- [OSS Growth Hacker] Reorder CHANGELOG BREAKING entry to lead with Claude Code format parity and Windows CI win; move size disclaimer to conditional escape-hatch context -- current narrative ends on the size disclaimer, converting an upgrade enthusiasm moment into migration anxiety for every reader scanning the changelog before upgrading.
Architecture
classDiagram
direction LR
class ArchiveUtils {
<<helper>>
+MAX_ZIP_ENTRIES int
+MAX_ZIP_UNCOMPRESSED int
+validate_archive_format(fmt)
+projected_archive_path(dir, name, fmt) Path
+write_zip_archive(bundle_dir, path)
+write_tar_archive(bundle_dir, path)
+safe_extract_zip(zf, dest, error_type) list
}
class Packer {
<<producer>>
+pack_bundle(root, out, archive, archive_format) PackResult
}
class PluginExporter {
<<producer>>
+export_plugin_bundle(root, out, archive, archive_format) PackResult
}
class Unpacker {
<<consumer>>
+unpack_bundle(path, out) UnpackResult
}
class LocalBundle {
<<consumer>>
+detect_local_bundle(path) LocalBundleInfo
-_extract_zip_bundle(path) LocalBundleInfo
}
class BuildOrchestrator {
<<orchestrator>>
+run(options, logger) BuildResult
}
class BuildOptions {
<<ValueObject>>
+bundle_archive bool
+bundle_archive_format str
}
class PackCommand {
<<command>>
+pack_cmd(fmt, archive, archive_format)
}
class RegistryExtractor {
<<consumer>>
+extract_archive(data, digest, dest) str
-_safe_extract_zip(zf, dest)
}
class RegistryClient {
<<network>>
+publish_version(archive_bytes) PublishResult
}
PackCommand ..> BuildOptions : creates
PackCommand ..> BuildOrchestrator : delegates
BuildOrchestrator ..> Packer : pack_bundle
BuildOrchestrator ..> PluginExporter : export_plugin_bundle
Packer ..> ArchiveUtils : write_zip/write_tar
PluginExporter ..> ArchiveUtils : write_zip/write_tar
Unpacker ..> ArchiveUtils : safe_extract_zip
LocalBundle ..> ArchiveUtils : safe_extract_zip
RegistryExtractor ..> ArchiveUtils : safe_extract_zip
RegistryClient ..> RegistryExtractor : pairs with
class ArchiveUtils:::touched
class Packer:::touched
class PluginExporter:::touched
class Unpacker:::touched
class LocalBundle:::touched
class BuildOptions:::touched
class PackCommand:::touched
class RegistryClient:::touched
class RegistryExtractor:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm pack --archive\n[--archive-format zip|tar.gz]"] --> B{"--archive-format\nwithout --archive?"}
B -- yes --> ERR["UsageError: has no effect\nwithout --archive"]
B -- no --> C["BuildOptions\nbundle_archive_format"]
C --> D["BuildOrchestrator"]
D --> F{"fmt?"}
F -- plugin --> G["export_plugin_bundle\narchive_format"]
F -- apm --> H["pack_bundle\narchive_format"]
G --> I{"archive?"}
H --> I
I -- no --> J["write bundle dir"]
I -- yes --> K{"archive_format?"}
K -- zip --> L["write_zip_archive\nname.zip\nZIP_DEFLATED compresslevel=9\nrglob skip symlinks"]
K -- tar.gz --> M["write_tar_archive\nname.tar.gz\ntarfile w:gz\nrglob skip symlinks"]
L --> P["shutil.rmtree bundle_dir"]
M --> P
subgraph extract ["apm install / apm unpack extraction"]
R["detect_local_bundle\nor unpack_bundle"] --> S{"extension?"}
S -- .zip --> T["safe_extract_zip\nerror_type=ValueError\nutils/archive.py"]
S -- .tar.gz --> U["_extract_tar_gz_file\nbackward compat"]
T --> V{"entries > limit?"}
V -- yes --> B1["ValueError: entry bomb"]
V -- no --> W{"symlink mode?"}
W -- yes --> B2["ValueError: symlink"]
W -- no --> X["stream chunks\ncumulative size check"]
X --> Y{"size > limit?"}
Y -- yes --> B3["ValueError: size bomb"]
Y -- no --> Z["chmod unix_mode & 0o755"]
end
Recommendation
PR #1720 merges correctly. The archive consolidation, format default switch, and security layering are all sound, and the Claude Code format alignment is a genuine strategic win. Two items must land before the next tagged release: the null-byte guard in _zip_member_target (with unit test) and the CLI integration test for apm install on a crafted zip. Both carry missing-evidence weight on secure-by-default surfaces -- the extraction security story is architecturally correct but the regression traps are absent, meaning the next routine refactor can silently reopen the path with no automated signal. The click.Choice case-sensitivity fix is a one-character change that should travel with the security patch in the same commit. The migration notice logging level and CHANGELOG narrative reorder are lower-urgency but should close in the same sprint to prevent the 'APM is noisy in CI' perception from forming before the format switch has time to establish itself in the community.
Full per-persona findings
Python Architect
-
[recommended] _raise typed -> None instead of -> NoReturn; dead code after bomb-limit calls invisible to type checkers at
src/apm_cli/utils/archive.py
archive.py defines_raise(error_type, message) -> Noneas a helper that always raises. Because typed -> None (not -> NoReturn), type checkers treat statements following each_raise(...)call as reachable. In safe_extract_zip, the lines after the bomb-limit _raise --fh.write(chunk)andtotal_uncompressed = next_total-- are unreachable at runtime but appear reachable to static analysis.
Suggested:from typing import NoReturnand annotatedef _raise(error_type: type[_ErrorT], message: str) -> NoReturn: raise error_type(message) -
[recommended] Fragile message-string inspection to classify ArchiveError in unpacker.py .tar.gz backward-compat path at
src/apm_cli/bundle/unpacker.py
if "path" in msg or "Symlinks" in msg or "links" in msgis brittle; both branches raise ValueError anyway. A future wording change to any ArchiveError message in archive.py silently breaks the classification without a test failure.
Suggested:except ArchiveError as exc: shutil.rmtree(temp_dir, ignore_errors=True); raise ValueError(str(exc)) from exc -
[nit] Redundant validate_archive_format calls in packer.py and plugin_exporter.py (Click already validates the value; projected_archive_path validates internally too)
-
[nit] Module-level requests.Session() creation eagerly couples all bundle callers to the network dependency at
src/apm_cli/utils/archive.py
Every pack/unpack/local-bundle import initializes a Session. Move to a lazy singleton_get_archive_session()accessor. -
[nit] Alias chain _MAX_UNCOMPRESSED_BYTES = MAX_ZIP_UNCOMPRESSED -> _MAX_ARCHIVE_DOWNLOAD_BYTES at
src/apm_cli/utils/archive.py
Download size and extraction size have different semantic roles; define as independent named constants.
CLI Logging Expert
-
[recommended] Migration notice fires permanently on every invocation -- no expiry or once-seen gate at
src/apm_cli/commands/pack.py
Everyapm pack --archivecall without--archive-formatemits the notice indefinitely. The--archivehelp text already carries the disclosure. Recommended: remove the runtime notice or gate it to a version-bound transition window. -
[recommended] Migration notice uses logger.info ([i]) instead of logger.warning ([!]) for a pipeline-breaking behavior change at
src/apm_cli/commands/pack.py
A format change from .tar.gz to .zip can silently break CI pipelines. [i] blue reads as low-priority; [!] yellow registers as "pay attention."
Suggested: Change tologger.warning("--archive now produces .zip by default. Use --archive-format tar.gz to restore the previous format for legacy pipelines.") -
[nit] Migration notice buried after per-file verbose list in --verbose mode; move if show_zip_migration_notice block before the for-f loop at
src/apm_cli/commands/pack.py -
[nit] First segment of UsageError is an f-string with no interpolation (remove the f prefix from the first segment) at
src/apm_cli/commands/pack.py
DevX UX Expert
-
[recommended] click.Choice is case-sensitive: --archive-format TAR.GZ silently fails with a confusing error at
src/apm_cli/commands/pack.py
Users typing TAR.GZ or Zip get "Invalid value for '--archive-format'" for a two-value enum. One-character fix.
Suggested:type=click.Choice(["zip", "tar.gz"], case_sensitive=False) -
[recommended] Migration notice fires on every invocation, not once -- noisy in CI at
src/apm_cli/commands/pack.py
No persistent seen-state. Options: write a seen-marker to a config dir, or emit as logger.warning so pipelines that suppress warnings can opt out. -
[recommended] --archive-format help text is ~250 chars -- wraps badly in a standard terminal at
src/apm_cli/commands/pack.py
Reference tools keep option help to ~120 chars. Size trade-off detail already appears in CHANGELOG and docs migration caution blocks. -
[nit] Migration notice appears after success line; emit before logger.success or upgrade to logger.warning for higher visual weight
-
[nit] Verbose tip leads with size benefit instead of CI-compat rationale; help text and PR body correctly lead with CI compatibility
Supply Chain Security Expert
-
[recommended] Null-byte zip entry filename bypasses the error_type parameter and leaks a raw ValueError to callers at
src/apm_cli/utils/archive.py
_zip_member_target has no null-byte guard; Path.resolve() raises plain ValueError before _raise() fires, violating the error_type API contract for ArchiveError callers including the download pipeline. No file is extracted so there is no security bypass, but the error_type API contract is broken and download pipeline error handling breaks for this input class.
Suggested:if "\x00" in member_name: _raise(error_type, f"Refusing to extract path with null byte: {member_name!r}")
Proof (missing at unit):tests/unit/utils/test_archive.py::test_extract_zip_rejects_null_byte_member_name-- proves: zip entry with null-byte filename rejected with caller-specified error_type [secure-by-default] -
[nit] No zip-path test for Windows drive-letter or UNC entry names despite PureWindowsPath guards at
tests/unit/utils/test_archive.py
tests/unit/bundle/test_tar_windows_absolute_path.py covers tar.gz; parallel tests needed for zip PureWindowsPath.drive checks. -
[nit] Zip extraction branch in unpacker.py uses bare except Exception instead of translating to ValueError like the tar.gz branch at
src/apm_cli/bundle/unpacker.py -
[nit] _extract_zip_bundle collapses mid-extraction OSError into silent None return at
src/apm_cli/bundle/local_bundle.py
Disk-full ENOSPC returns None instead of surfacing the real cause. Separate BadZipFile (-> None) from OSError (-> raise).
OSS Growth Hacker
-
[recommended] CHANGELOG BREAKING entry ends on size disclaimer -- narrative inversion converts upgrade enthusiasm into migration anxiety at
CHANGELOG.md
Current text ends: "ZIP archives are typically 30-130% larger..." Reorder to lead with Claude Code format parity and Windows CI win; move size note to conditional escape-hatch mention. -
[recommended] CI docs caution blocks frame the .zip switch as migration burden only -- miss the Windows CI win story at
docs/src/content/docs/integrations/ci-cd.md
Windows CI developers reaching these docs after a pipeline break see only the escape hatch. Prepend one-line positive framing: "ZIP archives extract natively on Windows runners without WSL or a tar binary and match the format Claude Code plugin hosts expect." -
[nit] Added/Changed CHANGELOG section ordering: escape hatch entry appears before the BREAKING change that motivates it
-
[nit] In-terminal migration notice has no pointer to docs for headless terminal operators
-
[nit] No pre-composed launch one-liner for release post: "APM bundles now speak Claude Code natively -- and unzip on any Windows runner without WSL."
Doc Writer
-
[recommended] ci-cd.md caution "instead of switching the restore step to unzip" implies fully migrating to .zip is wrong at
docs/src/content/docs/integrations/ci-cd.md
Rephrase to present both valid paths: add --archive-format tar.gz OR update the upload glob and restore step to .zip. -
[recommended] CHANGELOG Added entry for --archive-format and BREAKING Changed entry describe one behavioral unit but lack cross-reference at
CHANGELOG.md
A reader scanning only ### Added sees the flag without the motivating breaking change. Move the UsageError sentence into the Changed BREAKING entry or add a cross-reference. -
[nit] producer/index.md step 4 says "produces a .zip" but without --archive it produces a directory at
docs/src/content/docs/producer/index.md -
[nit] pack.md flag table --archive row buries migration guidance inside feature-description cell; migration caution block below the table already owns this at
docs/src/content/docs/reference/cli/pack.md -
[nit] pack-a-bundle.md mentions --archive-format tar.gz without cross-link to CLI reference at
docs/src/content/docs/producer/pack-a-bundle.md -
[nit] gh-aw.md does not clarify whether apm-action handles .zip natively or delegates to apm install at
docs/src/content/docs/integrations/gh-aw.md
Test Coverage Expert
-
[recommended] No test exercises apm install evil.zip to click.UsageError("Bundle security check failed: ...") through the CLI at
src/apm_cli/commands/install.py
Unit tests verify detect_local_bundle raises ValueError (passed, unit tier). No test exercises the CLI boundary. A future refactor in archive.py or install.py can silently reopen the path.
Proof (missing at integration-with-fixtures):tests/integration/test_install_local_bundle_e2e.py::test_install_zip_path_traversal_rejected_with_usage_error-- proves: apm install on a path-traversal zip exits non-zero and shows user-actionable error [secure-by-default] -
[recommended] export_plugin_bundle(archive=True, archive_format="tar.gz") section 15 has only dry-run test; no test verifies the actual .tar.gz file is written at
src/apm_cli/bundle/plugin_exporter.py
The live tar.gz branch in plugin_exporter.py section 15 is covered only by dry_run=True. Wrong argument order or transposed path would not be caught.
Proof (missing at unit):tests/unit/test_plugin_exporter.py::TestArchive::test_archive_tar_gz-- proves: apm pack --archive --archive-format tar.gz via plugin format produces readable .tar.gz [portability-by-manifest] -
[nit] _bundle_size_suffix() KiB and MiB formatting branches have no test at
src/apm_cli/commands/pack.py
Proof (missing at unit):tests/unit/commands/test_pack_cli_surface.py::TestBundleSizeSuffix::test_kib_formatting
Performance Expert
-
[recommended] sorted(rglob()) materializes full path list before first archive write -- streaming regression vs old tarfile.add() at
src/apm_cli/utils/archive.py
Old tarfile.add() did an internal os.walk-based streaming walk with no upfront materialization. sorted(rglob()) exhausts the generator and builds a list before any I/O; latency cliff at 5000+ files. Benefit: deterministic lexicographic order for bit-reproducible archives. Consider streaming heapq.merge variant if bundle sizes grow beyond a few thousand files. -
[recommended] compresslevel=9 in write_zip_archive costs 2-3x CPU for <3% ratio gain on text-heavy bundles at
src/apm_cli/utils/archive.py
Level 9 vs level 6: 2-5% smaller output, 2-3x longer compression CPU. For a 5MB skill bundle: ~75ms extra per pack invocation.
Suggested: Changecompresslevel=9tocompresslevel=6(zlib sweet spot, matches zip(1) default on most platforms) -
[recommended] ensure_path_within() resolves dest_root once per member even though dest_root is constant across extraction loop at
src/apm_cli/utils/archive.py
dest_root.resolve() called for every member: 10,000 wasted resolve() syscalls at max-entries limit = ~80ms overhead.
Suggested: Cacheresolved_base = dest_root.resolve()outside the loop in safe_extract_zip. -
[nit] ZIP extract chunk size increased from 64KB to 1MB on hot download path -- positive change, cuts Python loop iterations 16x for large members.
-
[nit] ZIP archives 30-130% larger than tar.gz for text-heavy bundles -- correctly documented in CHANGELOG, escape hatch exists, no action needed.
Auth Expert -- inactive
No auth surfaces touched. The only registry/client.py delta is Content-Type MIME type (application/gzip -> application/zip) and a cosmetic parameter rename -- neither affects how APM authenticates to any remote host.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1720 · sonnet46 22.4M · ◷
…ges (#1757) * chore: release v0.20.0 Bump pyproject.toml + uv.lock to 0.20.0 and roll the [Unreleased] CHANGELOG block into [0.20.0] - 2026-06-11. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.20.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(integration): refresh stale integration tests after v0.20.0 changes The release integration suite (which gates releases, not per-PR runs) surfaced 29 failures after v0.20.0 was tagged. Each was a stale test that had not been updated to match an intentional source behavior change merged this cycle. Source is correct throughout; only tests change here. Clusters and causing PRs: - A (#1735): download_github_file now calls _host._resolve_dep_auth_ctx; mock host helpers never stubbed it, forcing the wrong auth path. Added the stub to make_host/_make_host in 4 download test files. Also #1735 made generic raw-URL network errors raise RuntimeError instead of falling back to the Contents API -- rewrote 2 stale fallback tests to assert the raise. - B (#1742): apm compile -t copilot suppresses empty AGENTS.md shells when .github/instructions/*.md exists. Flipped 3 assertions to expect no AGENTS.md and assert the instruction files are present. - C (#1739): marketplace fetches moved to a shared requests.Session (_HTTP_SESSION.get); updated 6 tests to mock that seam. - D (#1734): optional registry env inputs are omitted without an override; updated 2 placeholder tests to assert the var is absent. - E (#1734): same omit-optional change; assert var not in result. - F (#1720): tar symlink rejection wording changed; updated the regex. Verified locally: 738 passed, 16 skipped (token-gated e2e). The two runnable cluster-B e2e tests (mixed_deps, guardrailing) pass against the v0.20.0 build; ado_e2e is identical logic, validated in CI. Full lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r changes (#1758) * chore: release v0.20.0 Bump pyproject.toml + uv.lock to 0.20.0 and roll the [Unreleased] CHANGELOG block into [0.20.0] - 2026-06-11. Lint mirror green locally (ruff check + format, pylint R0801, auth-signals). Post-merge: tag v0.20.0 to trigger the release workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(integration): refresh stale integration tests after v0.20.0 changes The release integration suite (which gates releases, not per-PR runs) surfaced 29 failures after v0.20.0 was tagged. Each was a stale test that had not been updated to match an intentional source behavior change merged this cycle. Source is correct throughout; only tests change here. Clusters and causing PRs: - A (#1735): download_github_file now calls _host._resolve_dep_auth_ctx; mock host helpers never stubbed it, forcing the wrong auth path. Added the stub to make_host/_make_host in 4 download test files. Also #1735 made generic raw-URL network errors raise RuntimeError instead of falling back to the Contents API -- rewrote 2 stale fallback tests to assert the raise. - B (#1742): apm compile -t copilot suppresses empty AGENTS.md shells when .github/instructions/*.md exists. Flipped 3 assertions to expect no AGENTS.md and assert the instruction files are present. - C (#1739): marketplace fetches moved to a shared requests.Session (_HTTP_SESSION.get); updated 6 tests to mock that seam. - D (#1734): optional registry env inputs are omitted without an override; updated 2 placeholder tests to assert the var is absent. - E (#1734): same omit-optional change; assert var not in result. - F (#1720): tar symlink rejection wording changed; updated the regex. Verified locally: 738 passed, 16 skipped (token-gated e2e). The two runnable cluster-B e2e tests (mixed_deps, guardrailing) pass against the v0.20.0 build; ado_e2e is identical logic, validated in CI. Full lint mirror green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(release): refresh release-validation harness for v0.20.0 behavior changes The release-validation shell harness carries its own copies of behavior assertions that duplicate the integration suite. Two of them went stale this cycle from the same PRs that broke the integration tests (#1757): - GH-AW compat (#1720): `apm pack --archive` now emits .zip by default; the archive check grepped only `build/*.tar.gz`. Accept either extension, testing each glob independently (a single `ls a b` exits non-zero when either pattern is unmatched, even if the other matches). - Hero scenario 2 / AGENTS.md (#1742): copilot `apm compile` omits the empty AGENTS.md shell when installed instructions already live under `.github/instructions/`. The check insisted AGENTS.md exist; now accept AGENTS.md OR a populated `.github/instructions/`, mirroring the merged pytest fix in test_guardrailing_hero_e2e.py. Same fixes applied to the Windows .ps1 (AGENTS.md only; it has no archive check). Predicates validated locally against apm v0.20.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sync the Stage 2 complexity/file-length refactor branch with main's 22 feature commits (Hermes #1726, Kiro IDE #1741, multi-host dep identity #1735, same-repo remote path deps #1732, git_file_transport #1740, revision pins #1738, marketplace sourceBase/source parity/inherit description #1736/#1739/#1755, pack --archive .zip #1720, mcp optional registry inputs #1734, and the v0.19.0/v0.20.0 releases). Conflict resolution preserved both sides: main's new features ported through the branch's extracted sibling modules, branch's tightened ruff thresholds (max-statements=120, max-branches=40, max-complexity=35, max-returns=8, max-args=12) and 800-line file limit retained. All 7 CI-mirror lint gates pass; full unit suite green (17099 passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
.tar.gzto.zipforapm pack --archive. ZIP is the format Claude Code and other plugin hosts expect, gives better Windows compatibility, and aligns withapm publish's auto-pack format.--archive-format [zip|tar.gz]so callers who need.tar.gz(existing CI pipelines, tooling that expects gzip) can opt in explicitly without a breaking change..tar.gzextraction toapm unpack/apm installso existing.tar.gzbundles continue to work after producers migrate to.zip.What changed
bundle/packer.pyimport tarfile; archive block branches onarchive_format; passes format toexport_plugin_bundlebundle/plugin_exporter.pyarchive_formatparam + branch in section 15bundle/unpacker.py.zipextraction path; legacy.tar.gzpath kept for backward compat; symlink detection viaexternal_attrfor zip entriescommands/pack.py--archivehelp updated; new--archive-format [zip|tar.gz]option (defaultzip)core/build_orchestrator.pybundle_archive_format: str = "zip"field inBuildOptions; threaded topack_bundleUsage
Test plan
pytest tests/unit/test_packer.py tests/unit/test_unpacker.py tests/integration/test_pack_unpack_e2e.py— all passpytest tests/unit/commands/test_pack_cli_surface.py tests/unit/commands/test_pack_phase3.py— all passapm pack --helpshows--archive-format [zip|tar.gz] [default: zip]apm pack --archiveproduces.zipapm pack --archive --archive-format tar.gzproduces.tar.gzapm install <bundle>.tar.gzstill works (backward compat)